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

Feature: allow fzf-lua to clear some env vars used by fzf, ripgrep, fd, ... #1266

Closed
1 task done
folke opened this issue Jun 14, 2024 · 15 comments
Closed
1 task done
Labels
feature request New feature

Comments

@folke
Copy link
Contributor

folke commented Jun 14, 2024

Have you RTFM'd?

  • I have done proper research

Feature Request

I've seen some obscure issues with fzf-lua that in the end were the result of the user having set an env var related to one of the tools used in fzf-lua.

One way to solve this on the LazyVim side, is to simply unset those variables, but that's not ideal, since then the env vars would also have been reset when using a regular terminal inside Neovim.

Better would be to simply not pass a given list to the shell commands using a setting.
Although, now that I think about it, I'm not even sure that will always work as expected. Probably depends on the shell and if it will source init files and as such re-populate the env again.

Possibly offending env vars:

  • FZF_DEFAULT_OPTS
  • RIPGREP_CONFIG_PATH

Any ideas?

Related: LazyVim/LazyVim#3656 (reply in thread)

@folke folke added the feature request New feature label Jun 14, 2024
@ibhagwan
Copy link
Owner

I’m unsure what to make of RIPGREP_CONFIG_PATH but FZF_DEFAULT_OPTS has been a contentious point, although it can conflict (had some preview issues, etc) users tend to use that a lot, especially those coming from an involved shell setup with fzf and they expect these settings to proxy so it’s not something we can remove, it’s also widely used with fzf.vim.

Ripgrep is different though, I think it’s worth considering temporary removal (by setting it in -/ null in the fzf spawn env).

@folke
Copy link
Contributor Author

folke commented Jun 14, 2024

Yeah, I wouldn't do that by default for sure, but just have an option to be able not to use FZF_DEFAULT_OPTS.

For ripgrep it might indeed make sense to ignore its vars by default.

@folke
Copy link
Contributor Author

folke commented Jun 14, 2024

The real issue is users that just add stuff to their shell, but have no idea what it does.
Then they see an issue and have no idea it's caused by some of the things they probably just copy-pasted from somewhere else in their dots.

So for those users, it actually does make sense to not use existing env vars, so I'd for sure enable that option in LazyVim 😅, but also document it so that advanced users can still opt out.

@ibhagwan
Copy link
Owner

ibhagwan commented Jun 14, 2024

For ripgrep it might indeed make sense to ignore its vars by default.

I tend to agree.

The real issue is users that just add stuff to their shell, but have no idea what it does.

l would like to give my users more credit, nevertheless your point is well made lol.

So for those users, it actually does make sense to not use existing env vars, so I'd for sure enable that option in LazyVim 😅, but also document it so that advanced users can still opt out.

But how will we differentiate the users who copy pasted willy nilly? They’d still have to open an issue and then have to enable the ignore_fz_defaulf_ops or whatever we call it unless we set it as default (which will create the opposite “my shell opts aren’t considered”).

How many cases like that exist that create bugs? I don’t think it’s too many as most opts are overwritten by fzf-lua anyways.

So far I’ve seen only one which I’ve tackled in #1107:

fzf-lua/lua/fzf-lua/fzf.lua

Lines 281 to 295 in ad4d70b

["FZF_DEFAULT_OPTS"] = (function()
-- Newer style `--preview-window` options in FZF_DEFAULT_OPTS such as:
-- --preview-window "right,50%,hidden,<60(up,70%,hidden)"
-- prevents our previewer from working properly, since there is never
-- a reason to inherit `preview-window` options it can be safely stripped
-- from FZF_DEFAULT_OPTS (#1107)
local default_opts = os.getenv("FZF_DEFAULT_OPTS")
if not default_opts then return end
local patterns = { "--preview-window" }
for _, p in ipairs(patterns) do
-- remove flag end of string
default_opts = default_opts:gsub(utils.lua_regex_escape(p) .. "[=%s]+[^%-]+%s-$", "")
-- remove flag start/mid of string
default_opts = default_opts:gsub(utils.lua_regex_escape(p) .. "[=%s]+.-%s+%-%-", " --")
end

@folke
Copy link
Contributor Author

folke commented Jun 14, 2024

Fair point... :)

Then maybe just the ripgrep one for now.

@ibhagwan
Copy link
Owner

Fair point... :)

Then maybe just the ripgrep one for now.

Alright I’ll handle it later when not AFK, unless it’s urgent for you, I can merge a PR adding:

["RIPGREP_CONFIG_PATH"] = '',

to the fzf process env vars:

fzf-lua/lua/fzf-lua/fzf.lua

Lines 277 to 280 in ad4d70b

env = {
["SHELL"] = shell_cmd[1],
["FZF_DEFAULT_COMMAND"] = FZF_DEFAULT_COMMAND,
["SKIM_DEFAULT_COMMAND"] = FZF_DEFAULT_COMMAND,

@folke
Copy link
Contributor Author

folke commented Jun 14, 2024

No, not urgent at all :)

Btw, I started working on a health.lua for fzf-lua, but there's a bug in Neovim health impl, that messes with the module name if it's in lua/fzf-lua/health.lua. It basically strips the .*lua/ part which is too much for fzf-lua.

Very annoying...

A simple healthcheck that checks for tool deps and warns when FZF_DEFAULT_OPTS etc is set would be a good solution too.

@ibhagwan
Copy link
Owner

No, not urgent at all :)

Btw, I started working on a health.lua for fzf-lua, but there's a bug in Neovim health impl, that messes with the module name if it's in lua/fzf-lua/health.lua. It basically strips the .*lua/ part which is too much for fzf-lua.

Very annoying...

A simple healthcheck that checks for tool deps and warns when FZF_DEFAULT_OPTS etc is set would be a good solution too.

Oh wow that’s a great idea too, ty @folke!

hopefully the upstream is fixed soon or I’ll take a stab at hacking it.

P.S. hope you did well on your sailing exam, I’d feel personally partially responsible if it’s due to neovim lo

@dotfrag
Copy link

dotfrag commented Jun 15, 2024

I'm a bit late to the party, but I'd still like to share my two cents :)

I've experienced this in LazyVim/LazyVim#3656 (comment) because I set the option quite a long time ago and I couldn't possibly remember. Another "solution" to this, is to explicitly override the conflicting options, which rg allows for most, if not all of them.

The benefit to this is that you can still respect the user's config, working in conjuction with fzf-lua's rg_opts. This also makes it obvious from the default rg_opts which option might cause an issue. The downside however, is that you have to magically guess for every option that might conflict, or wait for issues to appear...

I hope my comment doesn't give the impression that I'm trying to shove the responsibility to you, while this is clearly a user's (me) fault. Thank you both for responding and resolving this so quickly.

@ibhagwan
Copy link
Owner

The benefit to this is that you can still respect the user's config

That was mainly the reasoning behind keeping FZF_DEFAULT_OPTS, but IMHO this doesn’t apply to ripgrep, unlike bat which has a color scheme and layout preferences ripgrep in fzf-lua requires a specific format which can conflict with such options (as you experienced) so I’m having a hard time justifying why to keep ripgrep config var.

Can you provide a use case why it would be valuable to keep this var in thru context of fzf-lua.grep?

@dotfrag
Copy link

dotfrag commented Jun 15, 2024

I think there are a few cases but at the same time they don't affect me too much to make a strong argument. I've been using the latest implentation and I'm happy with it. The flags I've seen used mostly are -u, -uu to ignore hidden/gitignore and -F for fixed strings.

@ibhagwan
Copy link
Owner

The flags I've seen used mostly are -u, -uu to ignore hidden/gitignore and -F for fixed strings.

IMHO this just strengthened the case for nullifying the ripgrep options, search options should be controlled by fzf-lua only (rg_opts) as the results might start get confusing if you’ve set these and forgot.

@dotfrag
Copy link

dotfrag commented Jun 15, 2024

Perfect, I cannot think of other options on top of my head; solid case for nullifying then!

@ibhagwan
Copy link
Owner

@dotfrag, I guess there is a use case for RIPGREP_CONFIG_PATH, see #1288.

With f5240e3 you can now use:

require("fzf-lua").setup({
  grep = { RIPGREP_CONFIG_PATH = vim.env.RIPGREP_CONFIG_PATH }
})

@dotfrag
Copy link

dotfrag commented Jun 22, 2024

Thanks for keeping us in the loop!

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

No branches or pull requests

3 participants