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.

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

fixing the call to vim.fn.system #1550

Merged
merged 3 commits into from
Sep 25, 2022
Merged

Conversation

Xyhlon
Copy link
Contributor

@Xyhlon Xyhlon commented Aug 24, 2022

it is not desired to call vim.fn.system("whole command"), hence it takes all the output of the shell, there might be undesired info be parsed into the string aswell. see #1547 for more info.
better way of calling vim.fn.system({"whole","command"}) only this way the desired output of the command can be guarantied.

resolves #1547

@alex-courtis
Copy link
Member

Related: #1484

Prior art neotree: nvim-neo-tree/neo-tree.nvim@b30a418

Copy link
Member

@alex-courtis alex-courtis left a comment

Choose a reason for hiding this comment

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

Thanks for looking at this one.

Needs work.

Fix should also be applied to should_show_untracked

@@ -9,7 +9,7 @@ function M.get_toplevel(cwd)
local ps = log.profile_start("git toplevel %s", cwd)
log.line("git", cmd)
Copy link
Member

Choose a reason for hiding this comment

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

cmd is unused.

Please replace cmd with { "git", "-C", vim.fn.shellescape(cwd), "rev-parse", "--show-toplevel" }, log it and and pass it to vim.fn.system(

@@ -9,7 +9,7 @@ function M.get_toplevel(cwd)
local ps = log.profile_start("git toplevel %s", cwd)
log.line("git", cmd)

local toplevel = vim.fn.system(cmd)
local toplevel = vim.fn.system { "git", "-C", vim.fn.shellescape(cwd), "rev-parse", "--show-toplevel" }
Copy link
Member

Choose a reason for hiding this comment

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

This fails:

[2022-08-27 13:15:55] [profile] START git toplevel /home/alex/.local/share/nvim/vundle/nvim-tree.lua.Xyhlon
[2022-08-27 13:15:55] [git] git -C '/home/alex/.local/share/nvim/vundle/nvim-tree.lua.Xyhlon' rev-parse --show-toplevel
fatal: cannot change to ''/home/alex/.local/share/nvim/vundle/nvim-tree.lua.Xyhlon'': No such file or directory
[2022-08-27 13:15:55] [profile] END   git toplevel /home/alex/.local/share/nvim/vundle/nvim-tree.lua.Xyhlon  2ms

master:

[2022-08-27 13:15:28] [profile] START git toplevel /home/alex/.local/share/nvim/vundle/nvim-tree.lua.Xyhlon
[2022-08-27 13:15:28] [git] git -C '/home/alex/.local/share/nvim/vundle/nvim-tree.lua.Xyhlon' rev-parse --show-toplevel
/home/alex/.local/share/nvim/vundle/nvim-tree.lua.Xyhlon
[2022-08-27 13:15:28] [profile] END   git toplevel /home/alex/.local/share/nvim/vundle/nvim-tree.lua.Xyhlon  8ms
[2022-08-27 13:15:28] [profile] START git untracked /home/alex/.local/share/nvim/vundle/nvim-tree.lua.Xyhlon
[2022-08-27 13:15:28] [git] git -C /home/alex/.local/share/nvim/vundle/nvim-tree.lua.Xyhlon config status.showUntrackedFiles
[2022-08-27 13:15:28] [profile] END   git untracked /home/alex/.local/share/nvim/vundle/nvim-tree.lua.Xyhlon  7ms
[2022-08-27 13:15:28] [profile] START git job /home/alex/.local/share/nvim/vundle/nvim-tree.lua.Xyhlon nil
[2022-08-27 13:15:28] [git] running job with timeout 400ms
[2022-08-27 13:15:28] [git] git --no-optional-locks status --porcelain=v1 --ignored=matching -u
!! doc/tags
[2022-08-27 13:15:28] [git] done
[2022-08-27 13:15:28] [profile] END   git job /home/alex/.local/share/nvim/vundle/nvim-tree.lua.Xyhlon nil  4ms
[2022-08-27 13:15:28] [git] job success    /home/alex/.local/share/nvim/vundle/nvim-tree.lua.Xyhlon nil
log = {
      enable = true,
      truncate = true,
      types = {
        git = true,
        profile = true,
      },
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if you remove vim.fn.shellescape from cwd in the cmd everything works without the fatal error message

@alex-courtis
Copy link
Member

/nudge @Xyhlon

This will fix issues that many users are experiencing.

Copy link
Contributor Author

@Xyhlon Xyhlon left a comment

Choose a reason for hiding this comment

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

if you remove vim.fn.shellescape from cwd in the cmd everything works without the fatal error message

@@ -9,7 +9,7 @@ function M.get_toplevel(cwd)
local ps = log.profile_start("git toplevel %s", cwd)
log.line("git", cmd)

local toplevel = vim.fn.system(cmd)
local toplevel = vim.fn.system { "git", "-C", vim.fn.shellescape(cwd), "rev-parse", "--show-toplevel" }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

if you remove vim.fn.shellescape from cwd in the cmd everything works without the fatal error message

@alex-courtis
Copy link
Member

alex-courtis commented Sep 21, 2022

if you remove vim.fn.shellescape from cwd in the cmd everything works without the fatal error message

That sounds reasonable. It seems that cwd is being passed as an argument rather than to the command line, so no need to escape.

Tested a variety of cwd that include spaces and a variety of special characters e.g. # that would normally need to be escaped

I cannot test MacOS and Windows WSL, powershell etc, however I'm happy to release and see how things behave. The git runner passes the working directory in a similar manner so it should be fine.

@alex-courtis
Copy link
Member

I've merged master and applied the fix to toplevel, as well as untracked.

I cannot push to your repo. Please grant me access or merge master and apply
0001-fix-1547-pass-git-toplevel-cwd-unescaped-pass-git-un.patch.txt

@alex-courtis
Copy link
Member

I'm tempted to change the "msys2 git support" path, however the escaping seems fragile and I have no way to test it.

@alex-courtis
Copy link
Member

alex-courtis commented Sep 23, 2022

This should "resolve"
#1585
#1586
#1484
#549 (comment)
and others

We can also remove the fish workaround from https://github.com/kyazdani42/nvim-tree.lua#performance-issues

@Xyhlon
Copy link
Contributor Author

Xyhlon commented Sep 24, 2022

applied patch and access granted

@alex-courtis
Copy link
Member

Merged and pushed to your fork.

Tested a variety of cases with paths that we would normally escape; everything working as expected without vim.fn.shellescape.

Merging PR now.

@alex-courtis alex-courtis merged commit 9914780 into nvim-tree:master Sep 25, 2022
@alex-courtis
Copy link
Member

Many thanks for driving this change; this has been problematic for some time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Opening Directory in NVIMTREE causes ENAMETOOLONG error
2 participants