fix: bookmark filter shows marked directory children

[remvn]

Currently, bookmark filter doesn't include the children of marked directory

This PR fixes it

For now, this assume x is a directory by adding trailing slash approach is working fine. However, It would be better if I we knew the filetype of path and mark in the first place to skip some unnecessary checks (requiring changes from should_filter function). What do you think? Is it worth the change?

[alex-courtis]

However, It would be better if I we knew the filetype of path and mark in the first place to skip some unnecessary checks (requiring changes from should_filter function). What do you think? Is it worth the change?

Yes. That would be a lot faster and cleaner, which is desirable as this is a critical path function. We definitely don't want to make alny filesystem calls.

The node contains that information however we don't always have a node when calling this method, hence it is called with absolute path only.

Looking at the call sites we could add a uv.uv_fs_t parameter to should_filter(path, status). They appear to have it available one way or another.

[remvn]

I'm not sure what uv.uv_fs_t does (Is it used to do filesystem call to get the information?) but I think a new filetype argument as string should be good

[alex-courtis]

I'm not sure what uv.uv_fs_t does (Is it used to do filesystem call to get the information?)

That's it. It is returned by libuv filesystem calls https://github.com/luvit/luv/blob/master/docs.md#uvfs_statpath-callback and has a field .type which is a string. Example nvim-tree usage:

nvim-tree.lua/lua/nvim-tree/actions/fs/copy-paste.lua

Line 42 in 8f2a50f

if source_stats.type == "file" then

It's retained by the node

nvim-tree.lua/lua/nvim-tree/node.lua

Line 9 in 6a99f5a

---@field fs_stat uv.uv_fs_t

Node has a string type as well, however that's to identify the class of the node and cannot be used for our purposes.

All of the filter call sites have access to uv.uv_fs_t: from the node itself or the result of a stat e.g.

nvim-tree.lua/lua/nvim-tree/actions/finders/search-node.lua

Line 39 in 13f967f

stat, _ = vim.loop.fs_stat(path)

[remvn]

I think uv.uv_fs_t is not a table that carry information of the file but rather something to deal with async operation

see: https://github.com/luvit/luv/blob/master/docs.md#uvfs_statpath-callback

uv.fs_stat(path, [callback])
this fs_stat function actually has 2 different versions: sync and async

• async version (if we provide a callback): return a uv.uv_fs_t object which I think it's used to deal with other async operations
• sync version (only the path): return a table which contains the actual information we want. I believe it's uv.fs_stat.result from https://github.com/LuaCATS/luv/blob/3615eb12c94a7cfa7184b8488cf908abb5e94c9c/library/types.lua#L78-L96

Currently, only one should_filter call site doesn't have uv.fs_stat.result:

nvim-tree.lua/lua/nvim-tree/explorer/explore.lua

Lines 14 to 42 in e20966a

	---@param type_ string|nil
	---@param cwd string
	---@return any
	local function get_type_from(type_, cwd)
	return type_ or (vim.loop.fs_stat(cwd) or {}).type
	end
	---@param handle uv.uv_fs_t
	---@param cwd string
	---@param node Node
	---@param git_status table
	local function populate_children(handle, cwd, node, git_status)
	local node_ignored = explorer_node.is_git_ignored(node)
	local nodes_by_path = utils.bool_record(node.nodes, "absolute_path")
	local filter_status = filters.prepare(git_status)
	while true do
	local name, t = vim.loop.fs_scandir_next(handle)
	if not name then
	break
	end
	local abs = utils.path_join { cwd, name }
	local profile = log.profile_start("explore populate_children %s", abs)
	t = get_type_from(t, abs)
	if not filters.should_filter(abs, filter_status) and not nodes_by_path[abs] and Watcher.is_fs_event_capable(abs) then
	local child = nil
	if t == "directory" and vim.loop.fs_access(abs, "R") then

vim.loop.fs_scandir_next(handle) only provide name and type

Which means we will have to call vim.loop.fs_stat for every files in the loop

My current approach only uses type which available at all the places. There's really nothing useful in uv.fs_stat.result for filter purposes other than type

I believe it's ready to merge if there's nothing else to concern.

[alex-courtis]

I think uv.uv_fs_t is not a table that carry information of the file but rather something to deal with async operation

Right you are, that's a handle. The fs_stat field in node.lua is incorrect. It should indeed be a uv.fs_stat.result.

Many thanks for taking the time to identify this one.

[alex-courtis]

It looks like we can add uv.fs_stat.result as a param to filter rather than type. This is desirable so that we can in future filter on other criteria in that result set such as mtime.

RE explore.lua that is a problem. get_type_from doesn't usually (never?) execute the fs_stat. I agree that we should not blow out that loop by executing it. Node construction, however, does execute fs_stat.
Remediation:

• Remove get_type_from
• Replace get_type_from with an fs_stat
• Pass the stat to filter
• Pass the stat to the node constructor
  The above needs to be done one way or the other in the name of efficiency.

[alex-courtis]

If you'd like to take on the above task I would be most grateful, however I understand that this is outside the scope of this change.

If you choose not to we can merge this now and I'll raise an issue.

[alex-courtis]

In the interests of efficiency let's return early in the first line if bookmarks is empty or nil.

That will avoid the first utils.path_add_trailing

[alex-courtis]

FYI nvim 0.10 will be providing full luadoc api coverage including uv, lsp etc.

That would have made this PR much easier...

[remvn]

FYI nvim 0.10 will be providing full luadoc api coverage including uv, lsp etc.

That would have made this PR much easier...

Did you mean we will have type and intellisense for vim api functions?

[remvn]

Quick summary for few changes I have made:

• Node.fs_stat field are replaced with the correct type
• add fs_stat parameter to Node constructor
• Node constructor and should_filter function in explore.lua now will use the same fs_stat which is called once for every file in the loop.
• reload.lua: removed fs_stat_cached helper function, refactor some statement to make it consistent and cleaner

[alex-courtis]

Did you mean we will have type and intellisense for vim api functions?

Yes. I'm quite excited. It's anything vim.

[alex-courtis]

Nicely efficient.

Future PR: create a proper class for the return. It will be done when we apply all the 0.10 annotations and the enum is present.

[alex-courtis]

Many thanks!

[alex-courtis]

Just fantastic - thank you for going the extra mile with these great improvements to the codebase.

[alex-courtis]

Did you mean we will have type and intellisense for vim api functions?

#2731