Show active branch for recent repo #2005

mark2185 · 2022-06-17T19:11:37Z

This should close #1949.

It shows HEAD if the repo is in detached HEAD state, but I guess showing the branch name when not in detached HEAD state is better than nothing.

jesseduffield · 2022-07-01T07:35:00Z

I just tested this out locally and it took quite a while to render the menu when dealing with 76 recent repos (admittedly a lot but I'm sure others have more!).

We should prioritise speed here given that the typical use case for opening the recent repos menu is to quickly switch to another repo (often the first repo in the list). There are a few things we could consider doing:
1) spawn the git commands concurrently to get their results faster
2) render the list once without branch info and then re-render it once we have the branch info
3) get branch info ahead of time

I'm not really keen on option 3 just because it's one more thing we'd need to refresh in the background, and it sounds like it's pretty costly to keep things up to date. At the very least I think the two-phase rendering approach is a good idea.

As for rendering the menu, I'd have three separate columns: one for name, one for branch info, and one for path, in that order. Some repo paths are really long so it's better to have those on the end.

What are your thoughts?

Also, not necessarily related to this PR, but at some point it would be cool if our recent repos panel was a fully-fledged panel where there were keybindings for pulling/fetching. If we find it's hard to do the double-render thing with the menu panel, it might be easier to just create a standalone repos panel (which still appears as a popup)

mark2185 · 2022-07-01T18:14:07Z

I just tested this out locally and it took quite a while to render the menu when dealing with 76 recent repos (admittedly a lot but I'm sure others have more!).

I wonder which part exactly takes up so long. You wouldn't happen to be in the mood for profiling, would you? 🥺
The only time git was being noticeably slow is when I had a few gigabytes of data in git-lfs and zsh had trouble updating it's PS1, but that was due to zsh, tho.

At the very least I think the two-phase rendering approach is a good idea.

Out of the three it definitely sounds like the most reasonable. But I'm still a bit perplexed as to why is such a thing even necessary.

As for rendering the menu, I'd have three separate columns: one for name, one for branch info, and one for path, in that order. Some repo paths are really long so it's better to have those on the end.

What are your thoughts?

That was my initial approach, but I quickly realized I couldn't have three columns because all of the panels have 2 panels hardcoded, and this was a hack, basically. I'll do a more thorough analysis next time.

Also, not necessarily related to this PR, but at some point it would be cool if our recent repos panel was a fully-fledged panel where there were keybindings for pulling/fetching. If we find it's hard to do the double-render thing with the menu panel, it might be easier to just create a standalone repos panel (which still appears as a popup)

I mean I guess, but... I don't know, feels kinda icky fetching and pulling all willy-nilly from a different repo. I'll let it stew for a bit.

mark2185 · 2022-07-23T17:39:14Z

spawn the git commands concurrently to get their results faster

@jesseduffield how's this? Parallelized fetching the branches and storing it in sync.Map.

I tried it with 80 repos, but it was fast without it as well, and you have the proper benchmark so... would you kindly? :)

jesseduffield

Awesome work, I hadn't come across sync.Map before. I've left some more feedback

jesseduffield · 2022-07-24T23:00:21Z

pkg/utils/formatting.go

 	}
 	return slices.Map(lo.Range(maxWidth-1), func(i int) int {
 		return slices.MaxBy(stringArrays, func(stringArray []string) int {
+			// the `cancel` button has only one column so this avoids a crash if maxWidth > 2


I reckon that this function should be able to safely assume all the rows have the same number of columns. I would instead update the code that adds the cancel option to the menu to ensure it has the same number of columns as other rows.

Reason being that we want this function to catch bugs from elsewhere in the code that cause mismatched columns sizes

jesseduffield · 2022-07-24T23:01:57Z

pkg/gui/recent_repos_panel.go


+func (gui *Gui) getCurrentBranch(path string) string {
+	if branch, err := gui.os.Cmd.New(
+		fmt.Sprintf("git -C %s rev-parse --abbrev-ref HEAD", gui.os.Quote(path)),


I would add .DontLog() to this because we typically only log git commands that change something.

jesseduffield · 2022-07-24T23:04:55Z

pkg/gui/recent_repos_panel.go

 func (gui *Gui) handleCreateRecentReposMenu() error {
 	recentRepoPaths := gui.c.GetAppState().RecentRepos

+	currentBranches := sync.Map{}


that's one hell of an improvement! Nice work. Nonetheless, it still takes about a second on my end for 77 repos. Given that people typically expect menus to open instantly for the sake of muscle-memory (e.g. I might go to the status panel, hit enter then immediately hit enter again to go to the previous repo), I reckon we need to find a way to first render the menu without branches, then after it's been rendered, re-render it with the branches. I admittedly don't know how to do this off the top of my head but for all we know it's very straight-forward. I can dig into this deeper if you want.

Nonetheless, it still takes about a second on my end for 77 repos.

I'm really curious as to why is this the case. Is it always the same repo that's causing the issue or is something else amidst?

I reckon we need to find a way to first render the menu without branches, then after it's been rendered, re-render it with the branches.

That might look a bit janky if the branch name is the middle column, since we don't know the exact width of the longest branch, unless the second render shows both the branches and the paths.

I admittedly don't know how to do this off the top of my head but for all we know it's very straight-forward. I can dig into this deeper if you want.

I'd like to take a stab at it first then if I don't manage to find anything out I'll tug at your sleeve, deal? :)

I'm not sure which repo is the culprit, but even if it is a random repo causing the issue, I reckon it's better to just assume that some users are going to have their repo branches load slower than others, so we should try to make it smooth for everyone.

It might look a bit janky but I think it should be fine: users are typically going to be looking at the left column for the actual repo name so at least that will remain unchanged.

deal? :)

Deal :)

we should try to make it smooth for everyone.

Of course, couldn't agree more, I was just curious because I'm stumped, is all 😄

jesseduffield · 2022-07-24T23:05:17Z

pkg/gui/recent_repos_panel.go

+	wg := sync.WaitGroup{}
+	wg.Add(len(recentRepoPaths[1:]))
+
+	for _, path := range recentRepoPaths[1:] {


could we assign recentRepoPaths[1:] to a new variable so that we don't need to do the [1:] more than once?

But non-git directories should

Split recent repo menu into three columns

mark2185 · 2022-07-30T15:39:34Z

Nonetheless, it still takes about a second on my end for 77 repos.

Would you kindly test once more? :)

jesseduffield · 2022-07-31T00:39:26Z

@mark2185 THE ABSOLUTE MADMAN. Nice!!

jesseduffield · 2022-07-31T00:41:17Z

pkg/gui/recent_repos_panel.go

+		}
+	}
+
+	return "HEAD"


What does it actually mean if we reach here? Does it meant that there is no branch and the repo is also not on a detached head? It might be better to make it clear e.g. 'not on branch' or something

This means that:

there were either errors along the way (shouldn't happen, the paths are verified git repos for sure, unless reading the file fails)

or that repo is in detached HEAD state

I could put the short SHA here, actually.

yeah that works for me

My bad, it appears that currently the entire SHA was printed instead of HEAD.

So, HEAD will appear only if there's an error with reading the file.

But I presume we still don't need the entire SHA, right?

If we know that it is indeed a SHA then yeah we can truncate it. There's a helper method for that that we use when rendering commits.

If there's an error reading a file we're better off saying 'unknown branch'

There's a helper method for that that we use when rendering commits.

I think fileContent[:8] should suffice? It just contains the hash.

If there's an error reading a file we're better off saying 'unknown branch'

Good point. I18n needed?

I18n needed?

Just this tidbit remains, but I presume you're gonna say "Yes, of course!" :)

ah yes, of course! i18n is indeed needed

pkg/gui/recent_repos_panel.go

jesseduffield · 2022-07-31T00:44:13Z

pkg/app/app.go


 func isDirectoryAGitRepository(dir string) (bool, error) {
 	info, err := os.Stat(filepath.Join(dir, ".git"))
-	return info != nil && info.IsDir(), err


Are we removing this because the path may not actually be a directory? If so we should change the function / arg names

The arg is the path to the repo which is definitely a dir.

The <dir>/.git may be a:

directory ( ordinary git repository )

file ( an additional worktree git repository )

It's a repo whichever way you look at it though.

ah cool, that makes sense

pkg/gui/recent_repos_panel.go

jesseduffield · 2022-07-31T09:20:43Z

Code looks good to me

jesseduffield · 2022-07-31T09:30:41Z

great work 🚀

mark2185 requested a review from jesseduffield July 24, 2022 14:27

jesseduffield requested changes Jul 24, 2022

View reviewed changes

mark2185 added 3 commits July 30, 2022 17:03

Worktrees should not be filtered out

9c6239d

But non-git directories should

Show active branch for recent repo

e6e4513

Split recent repo menu into three columns

Parallelize fetching current branch

7c09ce3

mark2185 added 2 commits July 30, 2022 17:42

Gotta go fast

25ddac0

Refactor a bit, enable worktrees

9667332

jesseduffield reviewed Jul 31, 2022

View reviewed changes

pkg/gui/recent_repos_panel.go Outdated Show resolved Hide resolved

jesseduffield reviewed Jul 31, 2022

View reviewed changes

pkg/gui/recent_repos_panel.go Outdated Show resolved Hide resolved

jesseduffield reviewed Jul 31, 2022

View reviewed changes

Use filepath.Join instead of manual concat

767ef31

jesseduffield reviewed Jul 31, 2022

View reviewed changes

pkg/gui/recent_repos_panel.go Outdated Show resolved Hide resolved

mark2185 added 2 commits July 31, 2022 08:51

Display short SHA when in detached HEAD state

37bdbd9

Add i18n for unknown branch

44de380

jesseduffield merged commit 5f4c29d into jesseduffield:master Jul 31, 2022

mark2185 deleted the feature/recent-repos-path branch July 31, 2022 09:33

Uh oh!

Show active branch for recent repo #2005

Show active branch for recent repo #2005

Uh oh!

Conversation

mark2185 commented Jun 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jesseduffield commented Jul 1, 2022

Uh oh!

mark2185 commented Jul 1, 2022

Uh oh!

mark2185 commented Jul 23, 2022

Uh oh!

jesseduffield left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mark2185 commented Jul 30, 2022

Uh oh!

jesseduffield commented Jul 31, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mark2185 Jul 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jesseduffield commented Jul 31, 2022

Uh oh!

jesseduffield commented Jul 31, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mark2185 commented Jun 17, 2022 •

edited

Loading

mark2185 Jul 31, 2022 •

edited

Loading