ibhagwan / fzf-lua

Improved fzf.vim written in lua
GNU Affero General Public License v3.0
1.95k stars 133 forks source link

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

Closed folke closed 3 weeks ago

folke commented 3 weeks ago

Have you RTFM'd?

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:

Any ideas?

Related: https://github.com/LazyVim/LazyVim/discussions/3656#discussioncomment-9778646

ibhagwan commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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: https://github.com/ibhagwan/fzf-lua/blob/ad4d70b4658bcaad1e97c7879b32980c5244150c/lua/fzf-lua/fzf.lua#L281-L295

folke commented 3 weeks ago

Fair point... :)

Then maybe just the ripgrep one for now.

ibhagwan commented 3 weeks ago

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:

https://github.com/ibhagwan/fzf-lua/blob/ad4d70b4658bcaad1e97c7879b32980c5244150c/lua/fzf-lua/fzf.lua#L277-L280

folke commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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

I've experienced this in https://github.com/LazyVim/LazyVim/discussions/3656#discussioncomment-9778615 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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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

ibhagwan commented 2 weeks ago

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

With https://github.com/ibhagwan/fzf-lua/commit/f5240e364df3d3b954205aecf0c860cd7b185c06 you can now use:

require("fzf-lua").setup({
  grep = { RIPGREP_CONFIG_PATH = vim.env.RIPGREP_CONFIG_PATH }
})
dotfrag commented 2 weeks ago

Thanks for keeping us in the loop!