Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: handle windows file paths as uris #2640

Merged
merged 2 commits into from
Aug 10, 2023
Merged

fix: handle windows file paths as uris #2640

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a49d6cc
fix: handle windows file paths as uris
TM10YMhp Aug 7, 2023
8fc8f76
nit: rename FILE_PATH_PATTERN to WINDOWS_ROOT_PATTERN
TM10YMhp Aug 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion lua/telescope/utils.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,8 +174,11 @@ utils.is_path_hidden = function(opts, path_display)
end

local URI_SCHEME_PATTERN = "^([a-zA-Z]+[a-zA-Z0-9.+-]*):.*"
local WINDOWS_ROOT_PATTERN = "^[a-zA-Z]:\\"
utils.is_uri = function(filename)
return filename:match(URI_SCHEME_PATTERN) ~= nil
local is_uri_match = filename:match(URI_SCHEME_PATTERN) ~= nil
local is_windows_root_match = filename:match(WINDOWS_ROOT_PATTERN)
return is_uri_match and not is_windows_root_match
Comment on lines +180 to +181
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we dont need to do the is_windows_root match every time, we only have to do it if is_uri_match is valid

So we can improve performance by doing return is_uri_match and not filename:match(WINDOWS_ROOT_PATTERN)

also isn't there a better is_uri_match that allows us to only match once, idk there is an rfc for this thing: https://www.rfc-editor.org/rfc/rfc3986#page-17 and we are doing the following in plenary https://github.com/nvim-lua/plenary.nvim/blob/master/lua/plenary/path.lua#L75-L77

Copy link
Contributor

@jamestrew jamestrew Aug 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can clean up the unnecessary is_windows_root check.

I've don't think there's a clean lua pattern matching expression that can correctly identify a URI without over matching regular paths in one go. The plenary function is not correct either. Fails for any URI without //.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran some benchmarks against roughly 80k paths (windows and linux each) for the this implementation versus short circuiting the is_windows_root check and it's not a significant difference.

Details 

/tmp/testing ❯ hyperfine 'luajit current_linux.lua' 'luajit one_match_linux.lua' --warmup 10
Benchmark 1: luajit current_linux.lua
  Time (mean ± σ):      26.6 ms ±   0.8 ms    [User: 22.9 ms, System: 4.0 ms]
  Range (min … max):    25.4 ms …  30.4 ms    93 runs

Benchmark 2: luajit one_match_linux.lua
  Time (mean ± σ):      25.2 ms ±   0.8 ms    [User: 21.7 ms, System: 3.7 ms]
  Range (min … max):    24.0 ms …  27.8 ms    96 runs

Summary
  luajit one_match_linux.lua ran
    1.06 ± 0.04 times faster than luajit current_linux.lua


/tmp/testing ❯ hyperfine 'luajit current_win.lua' 'luajit one_match_win.lua' --warmup 10
Benchmark 1: luajit current_win.lua
  Time (mean ± σ):      36.7 ms ±   0.8 ms    [User: 33.5 ms, System: 3.5 ms]
  Range (min … max):    35.1 ms …  39.4 ms    69 runs

Benchmark 2: luajit one_match_win.lua
  Time (mean ± σ):      36.7 ms ±   0.8 ms    [User: 33.4 ms, System: 3.6 ms]
  Range (min … max):    35.6 ms …  39.5 ms    69 runs

Summary
  luajit one_match_win.lua ran
    1.00 ± 0.03 times faster than luajit current_win.lua

If speed is priority, we can switch to scanning the path byte by byte linearly.

Details 

hyperfine 'luajit current_linux.lua' 'luajit one_match_linux.lua' 'luajit fast_linux.lua' --warmup 10
Benchmark 1: luajit current_linux.lua
  Time (mean ± σ):      24.4 ms ±   0.9 ms    [User: 20.8 ms, System: 4.0 ms]
  Range (min … max):    22.9 ms …  27.4 ms    92 runs

Benchmark 2: luajit one_match_linux.lua
  Time (mean ± σ):      22.8 ms ±   0.9 ms    [User: 19.8 ms, System: 3.4 ms]
  Range (min … max):    21.1 ms …  26.1 ms    89 runs

Benchmark 3: luajit fast_linux.lua
  Time (mean ± σ):      21.3 ms ±   0.9 ms    [User: 17.8 ms, System: 3.8 ms]
  Range (min … max):    20.0 ms …  25.6 ms    104 runs

Summary
  luajit fast_linux.lua ran
    1.07 ± 0.06 times faster than luajit one_match_linux.lua
    1.15 ± 0.06 times faster than luajit current_linux.lua


/tmp/testing ❯ hyperfine 'luajit current_win.lua' 'luajit one_match_win.lua' 'luajit fast_win.lua' --warmup 10
Benchmark 1: luajit current_win.lua
  Time (mean ± σ):      36.3 ms ±   0.9 ms    [User: 33.5 ms, System: 3.2 ms]
  Range (min … max):    34.8 ms …  40.1 ms    70 runs

Benchmark 2: luajit one_match_win.lua
  Time (mean ± σ):      36.2 ms ±   1.1 ms    [User: 34.1 ms, System: 2.6 ms]
  Range (min … max):    34.5 ms …  41.2 ms    69 runs

Benchmark 3: luajit fast_win.lua
  Time (mean ± σ):      23.3 ms ±   0.8 ms    [User: 20.3 ms, System: 3.4 ms]
  Range (min … max):    22.0 ms …  26.2 ms    97 runs

Summary
  luajit fast_win.lua ran
    1.55 ± 0.07 times faster than luajit one_match_win.lua
    1.56 ± 0.06 times faster than luajit current_win.lua

This gives minor a performance boost for linux path and ~50% boost for windows paths.

see implementation #2648

end

local calc_result_length = function(truncate_len)
Expand Down