Skip to content

perf(#3171): use vim.system() instead of vim.fn.system() to execute git toplevel #3175

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

Merged

Conversation

przepompownia
Copy link
Collaborator

No description provided.

@przepompownia
Copy link
Collaborator Author

przepompownia commented Aug 4, 2025

This PR does not resolve #3171 - it only prevents cursor flickering in the case described there. Please notice unchanged win32 part - I cannot test it.

@przepompownia
Copy link
Collaborator Author

Pure Nvim example:

local timer = assert(vim.uv.new_timer())
timer:start(0, 600, function ()
  vim.schedule(function ()
    vim.fn.system({'true'})
  end)
end)

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.

It looks like vim.system was added in 2023: https://github.com/neovim/neovim/blob/c0952e62fd0/runtime/lua/vim/_editor.lua

Unfortunately it's not present in neovim v0.9.x, hence we'll need to add backwards compatibility shims. See example:

if vim.fn.has("nvim-0.10") == 1 then

@alex-courtis
Copy link
Member

Alternatively, I'm open to bumping the minimum neovim version to 0.11. Do you reckon it's time @gegoune ? We'd be at n-1 released nvim released version.

It would involve:

  • updating the minimum version check at startup
  • removing backwards compatibility shims for 0.9
  • updating readme/help/any other doc

That would be best done in a separate PR first.

@alex-courtis
Copy link
Member

Please notice unchanged win32 part - I cannot test it.

I think that should be OK, as vim's abstraction layer should handle it.

Can you confirm that out is the same before and after.

@przepompownia
Copy link
Collaborator Author

@przepompownia przepompownia changed the title fix(#3171): use vim.system() to determine git toplevel fix(#3171): use vim.system() to execute git commands Aug 5, 2025
@alex-courtis
Copy link
Member

0.11 is current stable
0.10 was released 2024-05-16
0.9 on 2023-04-07
0.8 on 2022-09-30

We moved to minimum version 0.9 on 2024-06-09

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.

Works on both codepaths, many thanks for the fix @przepompownia

@alex-courtis alex-courtis changed the title fix(#3171): use vim.system() to execute git commands perf(#3171): use vim.system() to execute git toplevel Aug 5, 2025
@alex-courtis alex-courtis changed the title perf(#3171): use vim.system() to execute git toplevel perf(#3171): use vim.system() instead of vim.fn.system() to execute git toplevel Aug 5, 2025
@alex-courtis alex-courtis merged commit 9a05b9e into nvim-tree:master Aug 5, 2025
7 checks passed
@przepompownia przepompownia deleted the git-toplevel-use-vim-system branch August 5, 2025 08:08
@gegoune
Copy link
Collaborator

gegoune commented Aug 5, 2025

Alternatively, I'm open to bumping the minimum neovim version to 0.11. Do you reckon it's time @gegoune ? We'd be at n-1 released nvim released version.

That would be best done in a separate PR first.

n-1 should be enough in cases where it makes it easier to maintain the code. If no breaking changes happened along the way we can extend it a bit, but more as a happy accident rather than our policy.

@alex-courtis
Copy link
Member

n-1 should be enough in cases where it makes it easier to maintain the code. If no breaking changes happened along the way we can extend it a bit, but more as a happy accident rather than our policy.

Cool, I guess we should document n-1 at the same 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.

3 participants