fix: infinite recursion in empty folder with hide_root_node = true

[figsoda]

fixes #714

the second commit is to avoid this from potentially happening in other edge cases

[miversen33]

It seems github won't let me comment on specific sections of a PR individually so I will wrap it all together.

commands.lua

• Pros: Less Recursion == gud

• Cons: Arbitrary loop limit may at some undefined point in the future be bad. That said, 256 seems fine, though below are some thoughts on this from windows and linux perspectives.

  • Windows has a max depth of 256 (if each directory is named a single character). As lua for loops are inclusive, this would account for that
  • Linux gets more fuzzy and could potentially be more than 256 dirs deep, depending on the file system. Because of this, it is possible (though unlikely) that the user is in a situation where their directory depth is more than the defined limit and they are navigating from some point more then 256 above the location they are looking for. Having this set as an arbitrary limit could cause undesired behavior with users such as that.
    Note: The above user situation is highly unlikely, but not impossible.

• Overall: IMO, it is a better decision to do an infinite loop (but not recursive) where we simply check to see if the current node pulled from the tree is the same as the previous node we pulled. If so, condition met, break the loop and we found the parent (because technically there is none). Also update the documentation, this is no longer doing recursion ;)

renderer.lua

• Pros: This does eliminate the issues with opening an empty directory with the root node hidden
• Cons: You are not doing what you think you are doing here. Your solution is completely bypassing the Nui Nodes that the rest of Neo-tree and any third party plugins use by simply setting a Nui Line with a message. This means that other extensions to Neo-tree (such as git signs, netman, etc) will have a line that says "Empty Folder" at the top of the window. . Note, the original line 1168 does indeed cause an issue here with a bug, but instead of going this route, its probably just better to do a bit of string checking at that point to ensure there is something to append to _empty_message (as this will fail with a string append to nil error currently).
• Overall: There is a less invasive way to do this update.

---

This PR can probably be compressed into a handful of lines. I would do a string check to ensure that state.path is something before adding it to _empty_message, and flip the for loop to a while true that breaks if the node has no parent (node:get_parent_id() returns nil if if has no parent), or if the node fetched node is the same as it was the previous pass over the loop (in addition to the other breaks in the loop).

Also, can you please update the merge target to nvim-neo-tree:v2.x? I believe we don't merge into main on this repo

[cseickel]

On the recursive loop topic, I would add these thoughts:

I think it is technically just looking for the closest parent which is a directory, so 256 is a very high number. The parents that are not directories are really just things like grouped files.

That being said, the correct way to fix this is to find the missing condition(s) that should break out of the recursion. Something like "if node is nil or node == prior_node".

Also, if we wanted a safety just in case we mess up the base case for the recursion, the line number of the buffer is technically the max possible number of levels that you could travel upwards in a tree. You could use that and it would work correctly as an escape hatch.

[figsoda]

commands.lua

Addressed

its probably just better to do a bit of string checking at that point to ensure there is something to append to _empty_message

There might be a bug with the original line 1168, but I don't think that's what's causing this bug. AFAIK the issue was either that skip_node removes the folder node completely so get_folder_node is unable to find the folder node, or that the _empty_message node was not a child of the folder node, so I let the parent node display the empty message and that fixed the issue. I tried altering the original _empty_message node (e.g. the snippet below) but all it ended doing was creating a new file in foo_empty_message/<file_name> instead of what should have been foo/<file_name>, I wonder if we should just make the id state.path instead and call that a day.

  if sourceItems then
    -- normal path
    if is_empty_with_hidden_root then
      local nodeData = {
        id = state.path .. "_empty_message",
        name = "(empty folder)",
        type = "message",
        level = 0,
        is_last_child = true,
      }
      local node = NuiTree.Node(nodeData, {})
      sourceItems[1].children = {node}
    end
    local nodes = create_nodes(sourceItems, state, level)
    draw(nodes, state, parentId)
  else

[figsoda]

Your solution is completely bypassing the Nui Nodes that the rest of Neo-tree and any third party plugins use

Would setting the type of the node to message here help resolve the issue? I couldn't figure out how to do that, the text just ended up with normal highlighting when I do item.type = "message" and return "(empty folder)"

[miversen33]

Ya setting the type to message should work. When you say

return "(empty folder)"

What do you mean?
I'm unsure we want to be overwriting the type that is passed in.

A side note, if we completely remove the changes that are being proposed to the renderer, your solution still works. I don't know that we need to be touching renderer at all (aside from the nil bug I mentioned earlier, though I can submit a PR for that if we want as its outside the scope of this issue).

[figsoda]

What do you mean?

with this patch, (empty folder) doesn't highlight as a message, probably because item.type = "message" doesn't do what I think it does. If there is some way to change the type to "message inside prepare_node, would it address your concerns about bypassing the Nui Nodes?

--- a/lua/neo-tree/ui/renderer.lua
+++ b/lua/neo-tree/ui/renderer.lua
@@ -317,9 +317,8 @@ end
 local prepare_node = function(item, state)
   if item.skip_node then
     if item.is_empty_with_hidden_root then
-      local line = NuiLine()
-      line:append("(empty folder)", highlights.MESSAGE)
-      return line
+      item.type = "message"
+      return "(empty folder)"
     else
       return nil
     end

if we completely remove the changes that are being proposed to the renderer, your solution still works

On my end, the hang is gone, but the created file ends up in foo_empty_message instead of foo. I am not experiencing the nil bug, if you can't reproduce this I can create a minimal reproducible example.

[miversen33]

It seems I got lost while looking at the PR, I was getting the changes in prepare_node and show_nodes mixed in my head lol. In this case, item.type isn't going to be used as I referenced above. Disregard my current complaint about prepare_node changes :)

[cseickel]

@miversen33 I'm not sure if you have permission to review or not. If you do have permission to approve and merge, I'll trust your review and you can proceed as you see fit.

[figsoda]

Also, can you please update the merge target to nvim-neo-tree:v2.x? I believe we don't merge into main on this repo

CONTRIBUTING.md seems to say otherwise: https://github.com/nvim-neo-tree/neo-tree.nvim/blob/v2.42/CONTRIBUTING.md#branching

Pull Requests, however, should go to the main branch

btw, should I keep a clean commit history and force push, or just keep all the commits?

[miversen33]

@figsoda Good catch! My bad :) I can't answer the second question, that is up to @cseickel

[cseickel]

btw, should I keep a clean commit history and force push, or just keep all the commits?

It's up to you. Most of the time I would just do a squash commit anyhow. If you had taken care to make a beautiful line up of well crafted commits with clean messages, I would rebase to preserve that, but that's really only needed in a large refactor.

Most PRs should just end up as a single squashed commit when merged, so I wouldn't change anything.

[figsoda]

sounds good, I will just keep the commit history then since it will still make sense in a single commit

[miversen33]

I think this hits all the points we discussed :) Thank you for both submitting the bug and addressing it with a PR!

[cseickel]

Thanks @figsoda and @miversen33! This looks good to me and I'll merge it now.