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

🆕 Command-T 6.0.0 release — compendium issue #393

Open
wincent opened this issue Aug 26, 2022 · 42 comments
Open

🆕 Command-T 6.0.0 release — compendium issue #393

wincent opened this issue Aug 26, 2022 · 42 comments

Comments

@wincent
Copy link
Owner

wincent commented Aug 26, 2022

Command-T 6.0.0

Now, this is a beta release (see |command-t-known-issues|), a total rewrite, subject to change, incompletely documented, with known bugs and feature gaps. But, because the entire world installs Neovim plugins by pointing at HEAD of main, people are probably going to end up trying this whether they intend to or not.

So, this feedback issue is to gather together common questions and bug reports. I also created this discussion thread for folks who prefer that format.

Information to include when reporting issues

  • What version of Neovim are you running? ie. nvim --version
  • What operating system are you running on? (eg. macOS Monterey, Arch Linux etc)
  • What system architecture? (eg. arm64, x86_64 etc)
  • How did you install Command-T? (eg. what package manager, if any, did you use?)
  • How have you configured Neovim and/or Command-T?
  • Do you have a minimal repro example, or a link to your dotfiles repo?
  • What other plug-ins do you have running, and have you tried reproducing the problem without them?

Summary

packer.nvim set-up

use {
  'wincent/command-t',
  run = 'cd lua/wincent/commandt/lib && make',
  setup = function ()
    vim.g.CommandTPreferredImplementation = 'lua'
  end,
  config = function()
    require('wincent.commandt').setup()
  end,
}

Via comment.

@wincent wincent pinned this issue Aug 26, 2022
This was referenced Aug 26, 2022
@wincent
Copy link
Owner Author

wincent commented Aug 29, 2022

Ok @deathmaz, @yujinyuz; seems reasonable; added to the default mappings in 1d40403.

@vheon
Copy link
Contributor

vheon commented Aug 31, 2022

If you try to run :CommandTGit in a directory which is not a git repo you get something like this

image

I guess in the old version it would fallback to the files scanner so now is a little uglier.

@wincent
Copy link
Owner Author

wincent commented Aug 31, 2022

Yep, this is a known issue @vheon. There is no fallback implemented yet for any of these things. Possible failure modes include:

  • Running :CommandTGit when git is not installed.
  • Running :CommandTRipgrep when rg is not installed.
  • Running :CommandTWatchman when watchman is not installed (or there is some other problem with Watchman daemon).

I had been postponing figuring out a solution for this because I wasn't sure what the best approach was. The problem is that the scanners all degrade "gracefully"; instead of returning errors, they effectively just return empty lists. As such, there are a number of main options, not necessarily mutually exclusive of one another:

  1. We teach the scanner_t type to include a status member that allows us to communicate something back to the callers.
  2. We teach the callers to interpret an empty result list as a cue to try again using a fallback mechanism; in an empty directory this would give a false negative but a relatively harmless one (ie. you might run :CommandTRipgrep in an empty folder, and Command-T would take that as a cue to try again with the standard scanner, which would of course also return an empty list).
  3. If we do "2", and maybe even if we don't, we may also want to swallow stderr so it doesn't leak back in any visible way (ie. that would mean changing a call to git ... into git ... 2> /dev/null etc). The only catch is that this may make trouble-shooting harder (users could edit out or change the redirect, I guess).
  4. We teach the callers to be a bit smarter before even trying to run a command that will definitely fail; eg. we might check to see if rg exists before trying to run it.

I haven't thought deeply on it, but I feel like "2" + "3" sound like a reasonable idea, for starters. We might later want to do "4". I don't find "1" very appealing at this stage.

wincent added a commit that referenced this issue Aug 31, 2022
This implements idea "3" from here:

- #393 (comment)

> we may also want to swallow `stderr` so it doesn't leak back in any
> visible way (ie. that would mean changing a call to `git ...` into
> `git ... 2> /dev/null` etc). The only catch is that this may make
> trouble-shooting harder (users could edit out or change the redirect,
> I guess).

So, yeah. This is a double-edged sword, but in the next commit I am
planning on dulling at least one of those edges by implementing a
fallback behavior for each of these scanners.
wincent added a commit that referenced this issue Aug 31, 2022
This implements idea "2" from here:

- #393 (comment)

> We teach the callers to interpret an empty result list as a cue to
> try again using a fallback mechanism; in an empty directory this would
> give a false negative but a relatively harmless one (ie. you might run
> `:CommandTRipgrep` in an empty folder, and Command-T would take that as
> a cue to try again with the standard scanner, which would of course
> also return an empty list).

Specifically, we do this whenever `git`, `rg`, or `find` returns no
results.

There is a bit of duplication here because we need thunks at each site;
otherwise as soon as we `require`-d the fallback finder it would do a
redundant scan. I'm not going to factor that out yet because I am still
not convinced that this is an optimal strategy. Want to let it bake for
a while and see what happens.
@wincent
Copy link
Owner Author

wincent commented Aug 31, 2022

I pushed 005b695 to implement a couple of those fallback ideas (specifically "2" and "3", like I said). Not sure if it's the best approach, but it's worth a shot. 🤷

@vheon
Copy link
Contributor

vheon commented Aug 31, 2022

My initial thoughts were simply to not let the error be displayed like that and maybe push a message through vim.notify, but before reading the code I thought that you were spawning the process from Lua through the job api and then send the candidates to the C code through the slab but I'm seeing now that your're calling the command in the C code directly so it wouldn't be that easy.

Anyway I'll give it a spin tomorrow at work, thanks for the quick response and fix!

@vheon
Copy link
Contributor

vheon commented Sep 5, 2022

@wincent before discovering the problem with scanners.git.untracked #405 I was puzzled because it was using the fallback to the find scanner but the title in the Prompt window was kept as git. Do you think command-t should signal somehow that the candidates I'm seeing are not coming from the scanner I asked for but that it fallback to something else?

@wincent
Copy link
Owner Author

wincent commented Sep 5, 2022

@vheon: I had thought about that while implementing the fallback, but didn't want to make any hasty decisions. Like, should the title include the word "fallback" to make it clear why the "wrong" scanner is being used? Would an :echomsg be better? Should it be displayed in some other way? I agree with you that we should say something, but I wasn't sure what the best way to do that might be.

@vheon
Copy link
Contributor

vheon commented Sep 5, 2022

In an ideal world probably a user might want the title to say something like:

CommandT [ find (fall-backed from git) ]

So that I know what I ended up with and what I actually wanted to with a vim.notify saying why the first scanner wasn't usable. This way the notification about the error can be read if found relevant but the fact that we fall-backed is kept persistent in the Prompt title.
Considering, however, that in a properly configured environment the fallback is rare to happen and that as you mentioned early the scanner is launched in the C code and we should have then a mechanism to pass the error to the Lua code I'm not sure if the whole notification is worth the trouble. I mean, yes it would be a much complete experience but the ROI isn't there I think.

wincent added a commit that referenced this issue Sep 5, 2022
Because we might be wanting to change the behavior soon, it would be
nice to be able to change in in _one_ place instead of three.

Specifically, as per:

- #393 (comment)

we might be putting something in the prompt window title to indicate
what happened.
wincent added a commit that referenced this issue Sep 5, 2022
As discussed here:

- #393 (comment)

That comment suggested being a bit more verbose:

    CommandT [ find (fall-backed from git) ]

but here I'm going with something a little less descriptive:

    CommandT [fallback]

seeing as the extra detail in the title doesn't tell you _what_ went
wrong; really, all we can do here is show a hint that _something_ went
wrong, so we might as well be brief about it.

Two things to note about implementation:

- One is that I am playing around here trying to figure out how much
  metatable magic I want in here without making it all too "clever". So
  here I have a `__newindex` implementation that allows us to write
  `prompt.name = 'fallback'` instead of `prompt:set_name('fallback')`.
  The former is visually cleaner, but I don't necessarily like that it
  hides the fact that an imperative side-effect is going to take place.
  I will probably "dumb this down" as opposed to trying to use the
  pattern in other places in a uniform way.

- The other is that my earlier claims about top-down React-style
  dataflow are pretty much not true any more. It _was_ true at the
  start, when each component rendered using data passed into it; now we
  have muddied the waters considerably with imperative methods. I will
  almost certainly want to revisit this topic in the future.
@wincent
Copy link
Owner Author

wincent commented Sep 5, 2022

@vheon Current proposal for communicating that a fallback happened is up in c16b272 — it just shows "fallback" in the end because I figure that we can't show complete diagnostic info in the title area, so there's not much value in saying more than that — it's just a cue that something didn't work. We can definitely looking at providing more detail via vim.notify() in the future though.

@vinitkumar
Copy link

@wincent Enjoying this plugin with my neovim. One small gripe(mostly because I couldn't find it in documentation), how do I make it ignore directories (e.g: node_modules)? It slows massively because it also indexes node_modules from my project.

@vinitkumar
Copy link

@wincent Enjoying this plugin with my neovim. One small gripe(mostly because I couldn't find it in documentation), how do I make it ignore directories (e.g: node_modules)? It slows massively because it also indexes node_modules from my project.

Alright, I think vim.cmd[[ let g:CommandTFileScanner = "git"]] is the way to go, or is there a better way for this?

@wincent
Copy link
Owner Author

wincent commented Sep 6, 2022

@vinitkumar: If you're using the older Ruby implementation, there are a number of ways to ignore files. Using the git scanner is one of them (that's what your command does). If you have/use Watchman, using the watchman scanner is another option (depending on how it is configured it may or may not index node_modules, but the whole point of Watchman is to be fast regardless). Another is to configure g:CommandTWildIgnore or Vim's own 'wildignore' setting (as described in command-t-wildignore).

In the newer Lua implementation, similar story — you can use :CommandTGit or :CommandTRipgrep (which will all ignore similar files), or :CommandTWatchman (which is supposed to be fast by design). In general, the Lua implementation should be faster anyway — perhaps fast enough that you don't care about the cost of indexing/searching in node_modules. I haven't (yet) added any ignore-related settings in the Lua implementation; I might if there is demand for them (but my preference is to keep things simple — using the Git finder is a simple way to search only "noteworthy" files).

@vinitkumar
Copy link

@vinitkumar: If you're using the older Ruby implementation, there are a number of ways to ignore files. Using the git scanner is one of them (that's what your command does). If you have/use Watchman, using the watchman scanner is another option (depending on how it is configured it may or may not index node_modules, but the whole point of Watchman is to be fast regardless). Another is to configure g:CommandTWildIgnore or Vim's own 'wildignore' setting (as described in command-t-wildignore).

In the newer Lua implementation, similar story — you can use :CommandTGit or :CommandTRipgrep (which will all ignore similar files), or :CommandTWatchman (which is supposed to be fast by design). In general, the Lua implementation should be faster anyway — perhaps fast enough that you don't care about the cost of indexing/searching in node_modules. I haven't (yet) added any ignore-related settings in the Lua implementation; I might if there is demand for them (but my preference is to keep things simple — using the Git finder is a simple way to search only "noteworthy" files).

I am using lua implementation. Perhaps, I will peek into your nvim config and see what you have done?

@wincent
Copy link
Owner Author

wincent commented Sep 6, 2022

@vinitkumar

I am using lua implementation. Perhaps, I will peek into your nvim config and see what you have done?

Ah, there is nothing to configure if you want to use the Git finder. Just invoke :CommandTGit instead of :CommandT. If you want to set up a mapping for that, something like this would do:

vim.keymap.set('n', '<Leader>t', '<Plug>(CommandTGit)')

This is (kind of) documented here.

@vheon
Copy link
Contributor

vheon commented Sep 13, 2022

@wincent I'm not sure if this is actually a bug or if simply I'm not used to the sorter engine of Command-T:

I'm "working" on my dotfiles (https://github.com/vheon/home) using the git scanner with this configuration
https://github.com/vheon/home/blob/137693a1866da57f026231b55b4423efdffee81b/roles/neovim/files/nvim/lua/plugins.lua#L175-L180

If in the root directory I run :CommandTGit and search for tmux this is what I get

image

but if I search for .tmux this is what I get

image

Is it on purpose?

@wincent
Copy link
Owner Author

wincent commented Sep 13, 2022

@vheon Yes, dotfiles are ignored by default. There are two settings that change this behavior:

With these defaults, you don't see the dotfiles unless your query includes the dot. With always_show_dot_files = true, you will see them regardless of the query. With never_show_dot_files = true, you won't see them even if your query includes a dot. Setting both to true doesn't make any sense, and Command-T will print an error if you try to do so.

@vheon
Copy link
Contributor

vheon commented Sep 13, 2022

As usual the RTFM would have solved my confusion without making noise here 😞 Thanks @wincent!

Padmamanickam added a commit to Padmamanickam/command-t that referenced this issue Feb 6, 2023
As reported here:

    wincent/command-t#393 (comment)

if `options.finders` is not set to (at least an empty) table, we freak
out with:

    lua/wincent/commandt/init.lua:343: bad argument #1 to 'pairs' (table expected, got nil)
Padmamanickam added a commit to Padmamanickam/command-t that referenced this issue Feb 6, 2023
This is still a private function and I won't be adding it to the docs as
officially supported API, but I want to provide this as a temporary
workaround for the problem reported here:

    wincent/command-t#393 (comment)

In short, the:

    https://github.com/jiangmiao/auto-pairs

has a bug in it because it hasn't been touched since February 2019, and
it wasn't written to account for the fact that Neovim mappings can now
map to Lua callbacks instead of Vimscript strings (ie. via the
`vim.keymap.set()` call). So, it has some code that does this:

    let info = maparg('<CR>', 'i', 0, 1)
    if empty(info)
      let old_cr = '<CR>'
      let is_expr = 0
    else
      let old_cr = info['rhs']
      " ... etc

It sees our `<CR>` mapping and dies because it assumes that the returned
`info` has a `'rhs'` key, but it doesn't:

    E716: Key not present in Dictionary: "rhs"

This commit, then, allows us to make a workaround by replacing the Lua
callback with a string-based mapping which _will_ have a RHS:

    require('wincent.commandt').setup({
      mappings = {
        i = {
          ['<CR>'] = "<C-o>:lua require('wincent.commandt.private.ui').open('edit')<CR>",
        },
      },
    })

ie. we replace the standard mapping, which maps to the internal `open()`
callback, with one that imperatively reaches in from the outside and
calls it as seen above.

Like I said, I won't be making this an official API, but I did want to
provide a workaround, seeing as it may be a while before auto-pairs gets
updated.
Padmamanickam added a commit to Padmamanickam/command-t that referenced this issue Feb 6, 2023
This is in response to this feedback:

    wincent/command-t#393 (comment)

The user in question is loading Command-T like this, using packer:

    use {
      'wincent/command-t',
      run = 'cd lua/wincent/commandt/lib && make',
      config = function()
        vim.g.CommandTPreferredImplementation = 'lua'
        require('wincent.commandt').setup({})
      end
    }

and they see this whenever they run `:PackerCompile`:

    commandt.setup():
      `commandt.setup()` was called after Ruby plugin setup has already run

So, I'm removing this check as it doesn't add a lot of value. My advice,
then, is to move this statement:

    vim.g.CommandTPreferredImplementation = 'lua'

up into the top-level of the `~/.config/nvim/init.lua`. That way,
whenever Command-T's `plugin/command-t.vim` is sourced, it will do the
right thing, which means:

1. Skip doing anything if `command_t_loaded` is already set.
2. Do Lua-centric set-up instead of Ruby-centric set-up.

And then when `plugin/command-t.lua` is sourced (which will always
happen second, because Neovim loads Lua _after_ Vimscript), it will
again do the right thing (ie. do the Lua-centric instead of Ruby-centric
set-up).
Padmamanickam added a commit to Padmamanickam/command-t that referenced this issue Feb 6, 2023
Padmamanickam added a commit to Padmamanickam/command-t that referenced this issue Feb 6, 2023
This implements idea "3" from here:

- wincent/command-t#393 (comment)

> we may also want to swallow `stderr` so it doesn't leak back in any
> visible way (ie. that would mean changing a call to `git ...` into
> `git ... 2> /dev/null` etc). The only catch is that this may make
> trouble-shooting harder (users could edit out or change the redirect,
> I guess).

So, yeah. This is a double-edged sword, but in the next commit I am
planning on dulling at least one of those edges by implementing a
fallback behavior for each of these scanners.
Padmamanickam added a commit to Padmamanickam/command-t that referenced this issue Feb 6, 2023
This implements idea "2" from here:

- wincent/command-t#393 (comment)

> We teach the callers to interpret an empty result list as a cue to
> try again using a fallback mechanism; in an empty directory this would
> give a false negative but a relatively harmless one (ie. you might run
> `:CommandTRipgrep` in an empty folder, and Command-T would take that as
> a cue to try again with the standard scanner, which would of course
> also return an empty list).

Specifically, we do this whenever `git`, `rg`, or `find` returns no
results.

There is a bit of duplication here because we need thunks at each site;
otherwise as soon as we `require`-d the fallback finder it would do a
redundant scan. I'm not going to factor that out yet because I am still
not convinced that this is an optimal strategy. Want to let it bake for
a while and see what happens.
Padmamanickam added a commit to Padmamanickam/command-t that referenced this issue Feb 6, 2023
Because we might be wanting to change the behavior soon, it would be
nice to be able to change in in _one_ place instead of three.

Specifically, as per:

- wincent/command-t#393 (comment)

we might be putting something in the prompt window title to indicate
what happened.
Padmamanickam added a commit to Padmamanickam/command-t that referenced this issue Feb 6, 2023
As discussed here:

- wincent/command-t#393 (comment)

That comment suggested being a bit more verbose:

    CommandT [ find (fall-backed from git) ]

but here I'm going with something a little less descriptive:

    CommandT [fallback]

seeing as the extra detail in the title doesn't tell you _what_ went
wrong; really, all we can do here is show a hint that _something_ went
wrong, so we might as well be brief about it.

Two things to note about implementation:

- One is that I am playing around here trying to figure out how much
  metatable magic I want in here without making it all too "clever". So
  here I have a `__newindex` implementation that allows us to write
  `prompt.name = 'fallback'` instead of `prompt:set_name('fallback')`.
  The former is visually cleaner, but I don't necessarily like that it
  hides the fact that an imperative side-effect is going to take place.
  I will probably "dumb this down" as opposed to trying to use the
  pattern in other places in a uniform way.

- The other is that my earlier claims about top-down React-style
  dataflow are pretty much not true any more. It _was_ true at the
  start, when each component rendered using data passed into it; now we
  have muddied the waters considerably with imperative methods. I will
  almost certainly want to revisit this topic in the future.
@LunarWatcher
Copy link

LunarWatcher commented Dec 24, 2023

Interesting [@]deathmaz. I haven't reproduced this yet, but my working theory is that it's happening here in auto-pairs:

let info = maparg('<CR>', 'i', 0, 1)
if empty(info)
  let old_cr = '<CR>'
  let is_expr = 0
else
  let old_cr = info['rhs']
  " ... etc

It sees the <CR> mapping that Command-T sets up (insert mode), and that mapping doesn't have 'rhs' because it's actually a Lua callback. Passing in Lua callbacks to Neovim's vim.keymap.set() API is legit, so this seems like a bug in auto-pairs.

@wincent For the record, you were mostly correct.

I stumbled into this independently a while ago (I maintain a fork of jiangmiao/auto-pairs, where the bug was triggered by lsp-zero), and opened an issue about it in the nvim repo. Strictly speaking, it's neovim that's at fault here. Yes, mapping Lua callbacks is valid, but Neovim mishandles how the callbacks are represented in a different API call, and also failed to document it. Nvim can either fix it to make the return values consistent, or document the actual behaviour of maparg() in nvim (the latter of which would be an incompatibility that's incredibly annoying to integrate into a plugin that isn't exclusively for nvim, but whatever; problem for future me if the issue ever gets handled).

Because an existing API function's return values isn't respected (nor is the outlier documented, making workarounds annoying on a good day), and lua function mappings don't return a value compatible with maparg()'s docs, the function explodes. Until the exact behaviour is documented (i.e. the behaviour is documented so the function can be considered stable again), or maparg is fixed, the bug is in Neovim, just elsewhere. I doubt it's going to be fixed any time soon though.

There are a few workarounds though, one of which you've already found; mapping it to something other than a lua function is fine, because that returns a valid value from maparg.

However, due to the nature of command-t, I'll argue disabling auto-pairs, or at least the cr map, in the command-t buffer is a good strategy. No mapping means no weird return values, and I doubt command-t benefits from the <cr> map in auto-pairs anyway.

With jiangmiao, I'd recommend running let b:autopairs_map_cr = 0 . If not in command-t itself, an autocmd FileType for the end-user also works fine. It just needs to be executed before auto-pairs is run1. Using let b:autopairs_loaded = 1 is also an option, and this fully prevents the plugin from loading.

My fork (which has the same problem) has a buffer filetype blacklist as well:

let g:AutoPairsFiletypeBlacklist = ["<insert command-T filetype here>"]

and works similarly to setting b:autopairs_loaded. The main advantage is that it can be set without waiting for the buffer to load, so no weird init order problems, if any exist. That said, my fork will complain loudly and explicitly (with some solutions) if rhs isn't present, so it's at least slightly more clear about the problem.

Anyway, welcome to the world of trying to map common keys (cr, space, tab, and backspace) in Vim - it sucks :p

Footnotes

  1. If command-t executes after auto-pairs, I assume it'll override <CR> instead, which is fine. The problem only appears when command-t, or any other plugin, maps a lua callback before auto-pairs tries to map <CR>. If the execution order is the other way around, it's not a problem.

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

No branches or pull requests

6 participants