Add root node in file tree

[stefanhaller]

• PR Description

If files at the root level of the repository have changed, there was no way to see the combined diff of all of them. Fix this by inserting a / item in this case. This is useful in repositories such as git.git, which have all their .c files at root level (gasp).

If only files inside some folder have changed, the root item gets compressed away automatically, so nothing changes for that case (which I guess should be more common for most people).

Addresses #4331.

• Please check if the PR fulfills these requirements

• 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)
• If a new UserConfig entry was added, make sure it can be hot-reloaded (see here)
• Docs have been updated if necessary
• You've read through your own file changes for silly mistakes etc

[ratorx]

Can confirm this is working well for me (have no opinions about implementation :))

It solves part of my problem, but I'll put more details about that in the discussion.

[stefanhaller]

I'm reasonably happy with the implementation now, and I cleaned up the commit history to make it reviewable. The distinction between GetPath and GetInternalPath might be a bit brittle and error-prone (easy to use the wrong one), but it's well-documented and the most reasonable solution I could think of. Let me know if you have any better ideas.

I wouldn't want to go about adapting all the tests (lots of them) before we have agreement about how the root item should look. Right now it's <root>, but does that look too ugly? Should it be just root? And does it have to be i18n-ized? I also considered making it just ., but maybe that's too subtle. But it would conceptually make sense; pressing space on it to stage all files would be equivalent to git add . on the command line.

[jesseduffield]

As discussed let's go with '/' for the root

[stefanhaller]

As discussed let's go with '/' for the root

Ok, force-pushed with that change if you want to try it more. Still no progress on adapting the tests, will do that on the weekend, probably.

[jesseduffield]

Thanks, will do

[jesseduffield]

Funnily enough I just hit an index.lock error when staging all files via the root file. It appears this is not in fact equivalent to pressing 'a', and I wonder if we should just use an explicit git add . (or whatever we do when we press 'a') upon pressing space on the root file. It's possible that index.lock error was just a freak accident, but I also assume there's a performance difference between the two approaches.

[stefanhaller]

I do think the index.lock error is unrelated. I also get this (very occasionally) when hitting space on non-root items.

The difference between space on the root item and a is git add -- . for the former, and git add -A for the latter. I have to read up on what difference the -A makes, but when hitting space on a subfolder we also use git add -- <path>, so I don't see why it should be any different at the root.

[jesseduffield]

Oh okay, good to know. Happy to consider it a coincidence.

[stefanhaller]

This is ready for review. I adapted the tests in separate commits for easier review, but these will have to be squashed into the commit that changes the behavior.

[codacy-production]

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
Report missing for e8d3a7a1	✅ 97.35%

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (e8d3a7a)	Report Missing	Report Missing	Report Missing
Head commit (2645952)	53486	46334	86.63%

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#4346)	565	550	97.35%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

_{Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more}

Footnotes

1. Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct. ↩

[jesseduffield]

Code looks good but after our recent convo relating to not soliciting feedback from enough people, I'm interested in knowing: if users don't like this change, how easy will be it be to make it opt-out (or opt-in if the backlash is sufficiently great)

[stefanhaller]

if users don't like this change, how easy will be it be to make it opt-out (or opt-in if the backlash is sufficiently great)

Not very hard; it would take a change roughly like this:

diff --git i/pkg/gui/filetree/build_tree.go w/pkg/gui/filetree/build_tree.go
index 1168b1a3d..e31164d13 100644
--- i/pkg/gui/filetree/build_tree.go
+++ w/pkg/gui/filetree/build_tree.go
@@ -7,14 +7,19 @@ import (
 	"github.com/jesseduffield/lazygit/pkg/commands/models"
 )
 
-func BuildTreeFromFiles(files []*models.File) *Node[models.File] {
+func BuildTreeFromFiles(files []*models.File, showRootNode bool) *Node[models.File] {
 	root := &Node[models.File]{}
 
 	childrenMapsByNode := make(map[*Node[models.File]]map[string]*Node[models.File])
 
 	var curr *Node[models.File]
 	for _, file := range files {
-		splitPath := split("./" + file.Path)
+		var splitPath []string
+		if showRootNode {
+			splitPath = split("./" + file.Path)
+		} else {
+			splitPath = split(file.Path)
+		}
 		curr = root
 	outer:
 		for i := range splitPath {
@@ -40,7 +45,7 @@ func BuildTreeFromFiles(files []*models.File) *Node[models.File] {
 				continue outer
 			}
 
-			if i == 0 && len(files) == 1 && len(splitPath) == 2 {
+			if showRootNode && i == 0 && len(files) == 1 && len(splitPath) == 2 {
 				// skip the root item when there's only one file at top level; we don't need it in that case
 				continue outer
 			}
@@ -70,12 +75,17 @@ func BuildFlatTreeFromCommitFiles(files []*models.CommitFile) *Node[models.Commi
 	return &Node[models.CommitFile]{Children: sortedFiles}
 }
 
-func BuildTreeFromCommitFiles(files []*models.CommitFile) *Node[models.CommitFile] {
+func BuildTreeFromCommitFiles(files []*models.CommitFile, showRootNode bool) *Node[models.CommitFile] {
 	root := &Node[models.CommitFile]{}
 
 	var curr *Node[models.CommitFile]
 	for _, file := range files {
-		splitPath := split("./" + file.Path)
+		var splitPath []string
+		if showRootNode {
+			splitPath = split("./" + file.Path)
+		} else {
+			splitPath = split(file.Path)
+		}
 		curr = root
 	outer:
 		for i := range splitPath {
@@ -94,7 +104,7 @@ func BuildTreeFromCommitFiles(files []*models.CommitFile) *Node[models.CommitFil
 				}
 			}
 
-			if i == 0 && len(files) == 1 && len(splitPath) == 2 {
+			if showRootNode && i == 0 && len(files) == 1 && len(splitPath) == 2 {
 				// skip the root item when there's only one file at top level; we don't need it in that case
 				continue outer
 			}

I'm personally pretty convinced they will like it though. 😄

[jesseduffield]

It's good to know it's not a big change to support that. I actually think in this case we should make it opt-out. That will mean that if people don't like it, there's a way out. I can imagine the extra keypress required to select a file (in the event where multiple top-level files/folders have changed) will annoy some people

[stefanhaller]

I actually think in this case we should make it opt-out.

I'm happy to add the config as soon as the first user complains, but I don't think we should add it proactively. I still believe there's a lot of value in keeping our config tidy and avoiding to add options that nobody uses. It's hard to know for sure, but I'd be very surprised if anybody will want to turn it off.

I can imagine the extra keypress required to select a file (in the event where multiple top-level files/folders have changed) will annoy some people

But the ability to see the combined diff of all files (which isn't possible at all right now) so outweighs this.

[jesseduffield]

Alright let's ship it and see what happens

[BijanRegmi]

is there any config to disable this?

[stefanhaller]

is there any config to disable this?

No, we discussed this above and decided to add one only once people complain.

So is your question a complaint? 😉

Seriously, can you explain why you are looking to turn this off?

[Zeglius]

Hi, I don't know if it's related, but now the unstaged changes panel doesn't show the diff of all the changes in the repo whenever the root node is selected, only can see changes per file if I select, or if I stage all the changes.

It is a bit of an annoyance, before it was fine and could have an overview of all the changes in, though I'm assuming this could be fixed without having to toggle off this root node thing? If so feel free to open a new issue (or just ping me so I open one myself).

Btw thank you all, I don't think I could have enjoyed git as much without lazygit ❤️

[Zeglius]

Hi, I don't know if it's related, but now the unstaged changes panel doesn't show the diff of all the changes in the repo whenever the root node is selected, only can see changes per file if I select, or if I stage all the changes.

It is a bit of an annoyance, before it was fine and could have an overview of all the changes in, though I'm assuming this could be fixed without having to toggle off this root node thing? If so feel free to open a new issue (or just ping me so I open one myself).

Btw thank you all, I don't think I could have enjoyed git as much without lazygit ❤️

Actually nvm ignore this, dumbass of me didn't commit the files, it does show up an overview of all the changes, my bad 😅

[stefanhaller]

@Zeglius Good. 😄

@BijanRegmi Any input from you?

[gwennlbh]

it's a bit weird to me because it basically sacrifices an entire identation level for something that can be achieved with 'a'

count me in as a vote to make in opt-out-able, but it's not that big of a deal honestly ^^

tbh: i was confused at first lol, I had changed stuff in the project's .vscode/ and i thought that lazygit had somehow found a .vsode/ at the root of my filesystem lmao

[stefanhaller]

sacrifices an entire indentation level

That's an interesting aspect that I hadn't considered. It doesn't bother me personally, but I can see how it could be a problem for people using very long file names, and/or having a small terminal window.

for something that can be achieved with 'a'

That's not the main reason why we added it; I agree that a is perfectly adequate for that, and I use it more often than pressing space on the root item. The real reason is so that you can see the whole diff of all your changed files at once.

[rizkyilhampra]

Hi. I was also looking for a way to turn off this behavior, but after reading the point about “being able to see the full diff of all changed files at once,” I think I’ll try to get used to it. Thanks, that makes sense now

[Dsaquel]

Thanks for this great feature 😀.

I think it would be nice having the the root node name configurable.