Add worktrees view #2147

kadaan · 2022-09-01T18:41:44Z

(Jesse speaking)

Adds a worktree view and operations for worktrees
If you don't use worktrees, you shouldn't notice any change other than the new worktrees tab appearing next to the files tab.

This is a first-cut. We will refine the UX as we go

jesseduffield · 2022-09-01T19:07:33Z

Wow! Didn't expect that you'd pick up on all the current patterns. Code looks pretty good so far, I'll try to find time on the weekend to give it a more thorough review

kadaan · 2022-09-01T20:19:30Z

Thanks! I have a few thoughts regarding worktree support

Does it make sense for worktrees to be a separate tab in branches panel? It feels like it should, but I am unsure.
In the branches list, I have filtered out branches that are checked out in a worktree that is not the current worktree.
What other operations from https://git-scm.com/docs/git-worktree would we want to support? I'm thinking about add and remove, in addition to list and switch, being a good first step.
We still need to handle worktrees which have been deleted from the filesystem, but not from git.
How do we want to handle worktree which are on removable devices (see git worktree lock ...)?
What information do we want to show on the right panel when a worktree is selected in the worktree list?
How do we want to display the current repo/worktree in the status panel? Currently it displays worktree_support -> worktree_support for the linked worktree. It would be better if it showed the repo, indicated we are in a worktree, and the branch.

pkg/commands/loaders/branches.go

mark2185 · 2022-09-02T08:26:05Z

I'll add my two cents.

Does it make sense for worktrees to be a separate tab in branches panel? It feels like it should, but I am unsure.

It does, IMO it's at the same level as submodules, as far as functionality and options go.

In the branches list, I have filtered out branches that are checked out in a worktree that is not the current worktree.

Maybe it would also be good to have them in the branches panel, but denote somehow that that branch is from a different worktree.

What other operations from https://git-scm.com/docs/git-worktree would we want to support? I'm thinking about add and remove, in addition to list and switch, being a good first step.

Sounds like a big step forward having just those few commands. We could always add more later.

We still need to handle worktrees which have been deleted from the filesystem, but not from git.

Just denote them somehow special and give the user an error when they try to switch to it?

How do we want to handle worktree which are on removable devices (see git worktree lock ...)?

Uhhh... how often is that used?

What information do we want to show on the right panel when a worktree is selected in the worktree list?

I guess we could do the same as the recent repositores popup does: folder-name current-branch-name path/to/repo ? Seems reasonable.

Sorta kinda related question, since it looks like you've used worktrees before - should we warn users that it will not work with git-lfs?

kadaan · 2022-09-02T14:16:41Z

That was my thought as well
I like the idea of using the branches panel. If we supported that, switching between branches that were checked out in different worktrees could etc worktree. Though, because linked worktrees can switch branches, we would need to support some way to checkout a branch into a worktree in addition to the current check out command. Lastly, we still would need the worktree panel, which means we would have multiple panels for managing worktrees. Sounds confusing…
Agreed
Another option is to just not show worktrees in the list if their target path does not exist
No idea, I never do
Good idea
Looks like LFS has linked worktree support: https://github.com/git-lfs/git-lfs/blob/main/docs/man/git-lfs-install.adoc. Am I missing something?

mark2185 · 2022-09-02T15:26:18Z

I like the idea of using the branches panel. If we supported that, switching between branches that were checked out in different worktrees could etc worktree. Though, because linked worktrees can switch branches, we would need to support some way to checkout a branch into a worktree in addition to the current check out command. Lastly, we still would need the worktree panel, which means we would have multiple panels for managing worktrees. Sounds confusing…

Well we can have both. Have the worktree panel for adding or removing worktrees, but still show all branches.

Another option is to just not show worktrees in the list if their target path does not exist

Or that, yeah.

Looks like LFS has linked worktree support: https://github.com/git-lfs/git-lfs/blob/main/docs/man/git-lfs-install.adoc. Am I missing something?

Huh, maybe it was a messup on my side. I tried using an additional worktree with several gigabytes of data in git-lfs and it didn't work out, I'll take a second look, thanks!

kadaan · 2022-09-02T15:58:23Z

@mark2185 There are a few things I'd like feedback on in this image

In the top left status panel, that is my quick attempt to differentiate being in a linked worktree vs the main work tree. Any other thoughts on how to do it?
In the worktrees panel, what should we call the main worktree. Initially, I chose "main", but the problem is I can create a worktree via git worktree add .worktrees/main which will then also be called "main". Also, I can create a worktree via git worktree add -b worktree_tests2 .worktrees2/worktree_tests which will be pointed at a new branch (two worktrees cannot check out the same branch), but will have the same final segment in the path. As you can see in the following gif, there are two worktrees and they have the same "name" in the worktrees panel because their final path segment is the same.
When icons are enabled, it helps differentiate the main and linked worktrees if they share a "name", but it can still get pretty confusing.
When icons are enabled I think it would be good to include a "link" icon for the linked worktrees in the worktree panel.
On the right status panel, I'm currently showing what we have decided it the "name" of the worktree and the path. Seems like the only things missing are the current branch name and maybe some indication that selected item is a main or linked worktree.

mark2185 · 2022-09-02T16:13:19Z

In the top left status panel, that is my quick attempt to differentiate being in a linked worktree vs the main work tree. Any other thoughts on how to do it?

That seems good enough, I've used that panel exactly 0 times so I'm fine either way.

In the worktrees panel, what should we call the main worktree. Initially, I chose "main", but the problem is I can create a worktree via git worktree add .worktrees/main which will then also be called "main".

Could we use the current working directory as it's name? As in, if the user just clones a repo, it'll be called e.g. lazygit?

Also, I can create a worktree via git worktree add -b worktree_tests2 .worktrees2/worktree_tests which will be pointed at a new branch (two worktrees cannot check out the same branch), but will have the same final segment in the path. As you can see in the following gif, there are two worktrees and they have the same "name" in the worktrees panel because their final path segment is the same.

That doesn't seem like a big issue. I don't know many people who use multiple worktrees so I don't have anyone to ask, but if I were creating multiple trees, I'd make sure they are not in folders of the same name.

When icons are enabled I think it would be good to include a "link" icon for the linked worktrees in the worktree panel.

Seems reasonable.

On the right status panel, I'm currently showing what we have decided it the "name" of the worktree and the path. Seems like the only things missing are the current branch name and maybe some indication that selected item is a main or linked worktree.

* is okay for the current worktree, the link icon is okay for the linked one, maybe we could use something tree-like for the main tree?

Also, I wouldn't hide invalid worktrees, I'd just mark them red or something. That way, the user can still delete them from within lazygit.

kadaan · 2022-09-02T16:17:05Z

Good points.

I have already added filtering out of the worktree, but I can add them back, style them red, and then check on enter to ensure the target location exists.

Also, I had to add back the check you called out in the Branches loader as it is needed to not filter out branches which are not checked out.

kadaan · 2022-09-02T16:34:16Z

mark2185 · 2022-09-02T16:37:07Z

Looks good! Does it also handle out-of-source worktrees just as well?

kadaan · 2022-09-02T17:29:05Z

What do you mean by "out of source"?

mark2185 · 2022-09-02T17:31:00Z

What do you mean by "out of source"?

All of these worktrees were created within your lazygit clone, right? What if you create one outside of it? I.e. git worktree add -b stable ../otherfolder or something similar.

kadaan · 2022-09-02T17:47:21Z

Yup, that works

Also, added a call out for missing worktrees on the right status panel

kadaan · 2022-09-02T17:58:50Z

I think the look of the top left status is a lot better like this

kadaan · 2022-09-02T20:32:17Z

Things remaining:

Support add worktree where branch name is worktree name
Support add worktree where branch name is different than worktree name
Add branch to worktree main panel display
Add option when deleting the worktree to delete the branch
Tests, tests, tests

kadaan · 2022-09-03T04:00:02Z

kadaan · 2022-09-03T04:37:56Z

Basic add support is in:

jesseduffield

Looking great so far. Left some feedback

pkg/commands/loaders/branches.go

pkg/commands/loaders/branches_test.go

pkg/commands/models/worktree.go

pkg/gui/refresh.go

kadaan · 2022-09-11T05:44:24Z

@jesseduffield Addressed all open comments

jesseduffield · 2022-09-17T19:11:30Z

pkg/commands/loaders/worktrees.go

+}
+
+func (self *WorktreeLoader) GetWorktrees() ([]*models.Worktree, error) {
+	worktreesOutput, err := self.cmd.New(`git worktree list --porcelain -z`).DontLog().RunWithOutput()


I'm running git 2.33 and looks like that doesn't support the -z flag, so I'd remove that and use newlines instead (assuming that doesn't present any major problems).
Speaking of compatibility, we have a compatibility requirement with git v2.0+, and it's worth considering if any of this PR doesn't work with git v2.0. I know magit supports as early as 2.4, so I'm happy to bump up the minimum required version to 2.4 if necessary for anything in this PR.

Based on 2 google searches, worktrees were introduced in v2.5, so...

shiiieeet haha

In that case I reckon we just need to only run our worktree code if we're on at least 2.5, otherwise we hide the worktree tab

jesseduffield · 2022-09-17T19:24:12Z

@kadaan thanks for waiting, I'm gonna give this another review this weekend

jesseduffield

I'm midway through a review but I need to submit it to respond to @mark2185 's comments

jesseduffield · 2022-09-17T19:12:49Z

pkg/gui/controllers/helpers/worktree_helper.go

+	return self.c.Prompt(types.PromptOpts{
+		Title: self.c.Tr.NewWorktreePath,
+		HandleConfirm: func(response string) error {
+			self.c.LogAction(self.c.Tr.Actions.CreateWorktree)


Can we wrap this in a loader, given it takes a couple seconds for me. Have a look at WithWaitingStatus

jesseduffield · 2022-09-17T19:14:25Z

pkg/gui/controllers/helpers/worktree_helper.go

+func (self *WorktreeHelper) IsCurrentWorktree(w *models.Worktree) bool {
+	pwd, err := os.Getwd()
+	if err != nil {
+		log.Fatalln(err.Error())


Can we add context to this error e.g. 'failed to determine a given worktree is the current worktree'. Given we're not panicking here, it's good to have something that's easy to search for within the codebase to see where it's failing.

jesseduffield · 2022-09-17T19:24:26Z

pkg/commands/loaders/worktrees.go

+}
+
+func (self *WorktreeLoader) GetWorktrees() ([]*models.Worktree, error) {
+	worktreesOutput, err := self.cmd.New(`git worktree list --porcelain -z`).DontLog().RunWithOutput()


shiiieeet haha

jesseduffield · 2022-09-17T19:24:56Z

pkg/commands/loaders/worktrees.go

+}
+
+func (self *WorktreeLoader) GetWorktrees() ([]*models.Worktree, error) {
+	worktreesOutput, err := self.cmd.New(`git worktree list --porcelain -z`).DontLog().RunWithOutput()


In that case I reckon we just need to only run our worktree code if we're on at least 2.5, otherwise we hide the worktree tab

jesseduffield · 2022-09-17T19:43:31Z

Some more things I noted from locally testing:

interestingly, a worktree named 'foo' will appear in git status --short as 'foo/'. Lazygit expects all status changes to be files, meaning it renders the workspace in the files view as a folder that has a file with no name. It would be good if we could actually signify that it's a worktree in the files panel, similar to what we do with submodules. Both when staged and unstaged it should appear as a single 'file' (right now it shows as a folder + file when unstaged and as a file when staged)
deleting a file that contains a worktree should refresh the worktree panel
When hiting space on a worktree, things get a bit weird: initially I'm taken to the files window, and then upon switching to the branches window again I'm kicked out of the Worktrees tab and put in the Local Branches tab. Then if I go back to the Worktrees and hit space on other worktrees some more, eventually it keeps me in the Worktrees tab but moves my cursor down by one item. We should be consistent in what happens. I think we should stay in the Worktrees tab and the cursor should remain on the item we just hit space on.
deleting worktree should also use a loader
hitting enter on a worktree should also make you enter it (not just space)

jesseduffield · 2022-09-17T19:44:44Z

pkg/gui/list_context_config.go

+			})
+		},
+		nil,
+		gui.withDiffModeCheck(gui.worktreesRenderToMain),


no need for the diff mode check: that's more applicable to things like branches/commits

jesseduffield · 2022-09-17T19:46:12Z

pkg/gui/presentation/icons/git_icons.go

@@ -59,3 +61,13 @@ func IconForRemote(remote *models.Remote) string {
 	}
 	return DEFAULT_REMOTE_ICON
 }
+
+func IconForWorktree(worktree *models.Worktree, missing bool) string {
+	if worktree.Main() {


I would show LINKED_WORKTREE_ICON even for the main worktree, so long as we also show the green asterisk

jesseduffield · 2022-09-17T19:46:33Z

pkg/gui/presentation/worktrees.go

+	"github.com/jesseduffield/lazygit/pkg/theme"
+)
+
+func GetWorktreeDisplayString(isCurrent bool, isPathMissing bool, worktree *models.Worktree) []string {


Suggested change

func GetWorktreeDisplayString(isCurrent bool, isPathMissing bool, worktree *models.Worktree) []string {

func GetWorktreeDisplayStrings(isCurrent bool, isPathMissing bool, worktree *models.Worktree) []string {

jesseduffield · 2022-09-17T19:48:32Z

pkg/gui/refresh.go

@@ -554,6 +571,10 @@ func (gui *Gui) refreshStatus() {

 	name := presentation.GetBranchTextStyle(currentBranch.Name).Sprint(currentBranch.Name)
 	repoName := utils.GetCurrentRepoName()
+	mainWorktreeName := gui.helpers.Worktree.GetMainWorktreeName()
+	if repoName != mainWorktreeName {
+		repoName = fmt.Sprintf("%s(%s)", mainWorktreeName, style.FgBlue.Sprint(repoName))


I'd use something other than blue given it can be hard to see on some screens (we typically use blue for unimportant stuff but I think it's important to show what worktree the user is in). How bout magenta?

Our min required git version is 2.20 so there's no need to add guards for worktrees because they were added in 2.5

We now always re-use the state of the repo if we're returning to it, and we always reset the windows to their default tabs. We reset to default tabs because it's easy to implement. If people want to: * have tab states be retained when switching * have tab states specific to the current repo retained when switching back Then we'll need to revisit this

This fixes pkg/integration/tests/worktree/rebase.go which was failing on old git versions due to a difference in order of branches that don't have recency values

We're doing all the IO in our workers loader method so that we don't need to do any in our presentation code

There are quite a few paths you might want to get e.g. the repo's path, the worktree's path, the repo's git dir path, the worktree's git dir path. I want these all obtained once and then used when needed rather than having to have IO whenever we need them. This is not so much about reducing time spent on IO as it is about not having to care about errors every time we want a path.

I would prefer to use methods to keep things immutable but I'd rather be consistent with the other models and update them all at once

We want to be using forward slashes everywhere internally, so if we get a path from windows we should immediately convert it to use forward slashes. I'm leaving out the recent repos list because that would require a migration

…nches When switching worktrees (which we can now do via the branch view) we re-layout the windows and their views. We had the worktree view ahead of the file view based on the Flatten() method in context.go, because it used to be associated with the branches panel.

Afero is a package that lets you mock out a filesystem with an in-memory filesystem. It allows us to easily create the files required for a given test without worrying about a cleanup step or different tests tripping on eachother when run in parallel. Later on I'll standardise on using afero over the vanilla os package

In the presentation layer, when showing branches, we'll show worktrees against branches if they're associated. But there was a race condition: if the worktree model was refreshed after the branches model, it wouldn't be used in the presentation layer when it came time to render the branches. A better solution would be to have some way of signalling that a particular context needs to be refreshed and after all the models are done being refreshed, we then refresh the contexts. This will prevent double-renders

mark2185 reviewed Sep 2, 2022

View reviewed changes

pkg/commands/loaders/branches.go Outdated Show resolved Hide resolved

kadaan force-pushed the worktree_support branch from efd8465 to 1609b4a Compare September 3, 2022 03:11

jesseduffield requested changes Sep 7, 2022

View reviewed changes

kadaan force-pushed the worktree_support branch from d73be3f to 6452caf Compare September 11, 2022 05:38

jesseduffield reviewed Sep 17, 2022

View reviewed changes

jesseduffield added 26 commits July 30, 2023 18:35

Fix unit tests

fb4453c

Remove worktree version guards

cdfad86

Our min required git version is 2.20 so there's no need to add guards for worktrees because they were added in 2.5

Update cheatsheets

18a508b

Add worktree integration tests

277142f

Add worktree tests for removing/detaching

b93b9da

Add more worktree tests

a313b16

Support fastforwarding worktree

b306006

Add test for retained context focus when switching worktrees

9c15ba0

Allow entering a worktree by pressing enter

bc4ce26

Allow entering a submodule by pressing space

e356b29

Fix test by making branches appear deterministically

0a41014

This fixes pkg/integration/tests/worktree/rebase.go which was failing on old git versions due to a difference in order of branches that don't have recency values

Remove dead function

e874f94

Add test for opening lazygit in the worktree of a bare repo

2b24c15

Remove IO logic from presentation code for worktrees

de57cfd

We're doing all the IO in our workers loader method so that we don't need to do any in our presentation code

Use fields rather than methods on worktrees

06be88a

I would prefer to use methods to keep things immutable but I'd rather be consistent with the other models and update them all at once

Add more i18n for worktrees

eecf07c

Add section in integration readme about testing against old git versions

a77d24f

Use forward-slashes on windows

b16bb40

We want to be using forward slashes everywhere internally, so if we get a path from windows we should immediately convert it to use forward slashes. I'm leaving out the recent repos list because that would require a migration

rename files

f31e213

Use double dash for disambuating path in editor templates

d30eef6

fix go mod

2d4706e

jesseduffield force-pushed the worktree_support branch from 3557bc4 to 2d4706e Compare July 30, 2023 08:36

jesseduffield merged commit 08f0e28 into jesseduffield:master Jul 30, 2023

stefanhaller mentioned this pull request Dec 17, 2023

Add remoteBranchOrder config #3158

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add worktrees view #2147

Add worktrees view #2147

kadaan commented Sep 1, 2022 •

edited by jesseduffield

Loading

jesseduffield commented Sep 1, 2022

kadaan commented Sep 1, 2022

mark2185 commented Sep 2, 2022

kadaan commented Sep 2, 2022

mark2185 commented Sep 2, 2022

kadaan commented Sep 2, 2022

mark2185 commented Sep 2, 2022

kadaan commented Sep 2, 2022

kadaan commented Sep 2, 2022

mark2185 commented Sep 2, 2022

kadaan commented Sep 2, 2022

mark2185 commented Sep 2, 2022

kadaan commented Sep 2, 2022

kadaan commented Sep 2, 2022

kadaan commented Sep 2, 2022 •

edited

Loading

kadaan commented Sep 3, 2022

kadaan commented Sep 3, 2022

jesseduffield left a comment

kadaan commented Sep 11, 2022

jesseduffield Sep 17, 2022

mark2185 Sep 17, 2022

jesseduffield Sep 17, 2022

jesseduffield Sep 17, 2022

jesseduffield commented Sep 17, 2022

jesseduffield left a comment

jesseduffield Sep 17, 2022

jesseduffield Sep 17, 2022

jesseduffield Sep 17, 2022

jesseduffield Sep 17, 2022

jesseduffield commented Sep 17, 2022

jesseduffield Sep 17, 2022

jesseduffield Sep 17, 2022

jesseduffield Sep 17, 2022

jesseduffield Sep 17, 2022

	func GetWorktreeDisplayString(isCurrent bool, isPathMissing bool, worktree *models.Worktree) []string {
	func GetWorktreeDisplayStrings(isCurrent bool, isPathMissing bool, worktree *models.Worktree) []string {

Add worktrees view #2147

Add worktrees view #2147

Conversation

kadaan commented Sep 1, 2022 • edited by jesseduffield Loading

jesseduffield commented Sep 1, 2022

kadaan commented Sep 1, 2022

mark2185 commented Sep 2, 2022

kadaan commented Sep 2, 2022

mark2185 commented Sep 2, 2022

kadaan commented Sep 2, 2022

mark2185 commented Sep 2, 2022

kadaan commented Sep 2, 2022

kadaan commented Sep 2, 2022

mark2185 commented Sep 2, 2022

kadaan commented Sep 2, 2022

mark2185 commented Sep 2, 2022

kadaan commented Sep 2, 2022

kadaan commented Sep 2, 2022

kadaan commented Sep 2, 2022 • edited Loading

kadaan commented Sep 3, 2022

kadaan commented Sep 3, 2022

jesseduffield left a comment

Choose a reason for hiding this comment

kadaan commented Sep 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jesseduffield commented Sep 17, 2022

jesseduffield left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jesseduffield commented Sep 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kadaan commented Sep 1, 2022 •

edited by jesseduffield

Loading

kadaan commented Sep 2, 2022 •

edited

Loading