Make "unpushed" take precedence over "merged" on main branch #2721
Conversation
When working on the main branch, make the unpushed (red) status of commits take precedence over merged (green), which would be otherwise applied by default to all commits.
jdujava
force-pushed
the
unpushed-over-merged-on-main
branch
from
e6dd658
to
22f9c10
Compare
There was a problem hiding this comment.
Ah, thanks for catching this mistake. I didn't notice when I made the change because I never work on master directly.
A few thoughts about the implementation below. However, before merging this we should think about other ways how this could be solved: I recently ran into the situation where my local master wasn't up to date, and I was working on a feature branch that a colleague had rebased onto origin/master. I was confused that the color-coding didn't show me where the branch started. I thought that this can be solved by also including the respective upstreams of the main branches in the merge-base call. However, then it occurred to me that maybe we only need the remote main branches in that call. This would have the advantage that color-coding of a branch against devel works even if you don't have a local devel branch yourself.
And if we make that change, it would solve this PR's issue automatically too. (I think -- haven't tried it yet.)
Thoughts?
| @@ -349,12 +349,25 @@ func (self *CommitLoader) setCommitMergedStatuses(refName string, commits []*mod | |||
| if ancestor == "" { | |||
| return commits | |||
| } | |||
| inMainBranch := false | |||
| branchRef, err := self.cmd.New( | |||
| NewGitCmd("symbolic-ref").Arg("HEAD"). | |||
There was a problem hiding this comment.
This is not quite right. It works for the commits panel, but the same code also runs for the commits of a branch if you press enter on it in the branches panel; in that case the branch isn't necessarily the same as the currently checked-out branch. But we still want the color-coding to work correctly in that case, too.
One way to solve this is to run the "symbolic-ref" command only if refName == "HEAD", but set branchRef to refName otherwise.
Maybe @jesseduffield can think of a nicer way of doing this; ideally we wouldn't have to make a git call at all here, but pass the branch name in somehow.
| if mainRef == branchRef { | ||
| inMainBranch = true | ||
| } | ||
| } |
There was a problem hiding this comment.
This loop can be shortened to a one-liner by saying inMainBranch = lo.Contains(self.mainBranches, branchRef).
BTW, I'd prefer the name isMainBranch, but that's a nitpick.
|
I'm not sure yet, but I feel that a fix for both problems might be as simple as something like this patch against master: diff --git a/pkg/commands/git_commands/commit_loader.go b/pkg/commands/git_commands/commit_loader.go
index 777f13d5c..cfae54408 100644
--- a/pkg/commands/git_commands/commit_loader.go
+++ b/pkg/commands/git_commands/commit_loader.go
@@ -392,7 +392,7 @@ func (self *CommitLoader) getMergeBase(refName string) string {
func (self *CommitLoader) getExistingMainBranches() []string {
return lo.FilterMap(self.UserConfig.Git.MainBranches,
func(branchName string, _ int) (string, bool) {
- ref := "refs/heads/" + branchName
+ ref := "refs/remotes/origin/" + branchName
if err := self.cmd.New(
NewGitCmd("rev-parse").Arg("--verify", "--quiet", ref).ToArgv(),
).DontLog().Run(); err != nil {Except I'm not sure that hard-coding "origin" is a good idea. I don't have time to play with this more today, but hopefully on the weekend. |
|
I completely agree with your comments! If you are willing to work on this, please take it away 😉. |
|
See #2725. |
|
Closed in favour of #2725. |
Dislaimer: I am not familiar with the codebase, nor with the
golanguage. It may be possible I missed something, and my implementation is probably flawed in some way. I will appreciate any help!After the change introduced in #2628, all the commits on the main branch were colored in green ("merged").
It seems natural to me, when working on the main branch, to make the "unpushed" (red) status of commits take precedence over the "merged" (green) status, which would be otherwise applied by default to all commits.
go run scripts/cheatsheet/main.go generate)docs/Config.md) have been updated if necessary