Skip to content

fix(git): make mark_ignored async #621

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
merged 3 commits into from
Dec 4, 2022
Merged

fix(git): make mark_ignored async #621

merged 3 commits into from
Dec 4, 2022

Conversation

musjj
Copy link
Contributor

@musjj musjj commented Nov 25, 2022

This PR introduces an alternative async interface for git.mark_ignored. By providing a callback to the function, it will run the git operations via luv.spawn instead of vim.fn.systemlist.
In the async interface, the files to be checked are fed through the stdin, instead of through the CLI arguments. This is because certain collection of filenames causes luv to fail to spawn the git process (maybe because of the length or certain unicode characters?). If there are any performance issues caused by the pipe method, I'll try to investigate further on it.

@musjj
Copy link
Contributor Author

musjj commented Nov 26, 2022

Just realized that some tests are failing, not sure what's causing this 🤔

@musjj
Copy link
Contributor Author

musjj commented Dec 1, 2022

@cseickel Sorry for bugging you again, but any idea why this is happening?
Apparently during the test, directories aren't being folded correctly when async_git_status is set to true, but I'm not experiencing this issue on my own setup.
I'm guessing that there's some kind of race condition because it's asynchronous?

This adds an arbitrary delay after sending the keys in order to wait for the tree to be fully expanded
@codecov
Copy link

codecov bot commented Dec 1, 2022

Codecov Report

Merging #621 (16382c8) into main (e003910) will increase coverage by 0.05%.
The diff coverage is 75.80%.

@@            Coverage Diff             @@
##             main     #621      +/-   ##
==========================================
+ Coverage   50.45%   50.50%   +0.05%     
==========================================
  Files          47       47              
  Lines        6099     6187      +88     
==========================================
+ Hits         3077     3125      +48     
- Misses       3022     3062      +40     
Impacted Files Coverage Δ
lua/neo-tree/sources/filesystem/lib/fs_scan.lua 71.49% <68.75%> (-0.96%) ⬇️
lua/neo-tree/git/ignored.lua 72.41% <78.26%> (-6.05%) ⬇️
lua/neo-tree/setup/init.lua 69.36% <0.00%> (-3.74%) ⬇️
lua/neo-tree/sources/buffers/init.lua 70.00% <0.00%> (-1.00%) ⬇️
lua/neo-tree/sources/manager.lua 60.79% <0.00%> (-0.31%) ⬇️
lua/neo-tree/utils.lua 47.07% <0.00%> (-0.25%) ⬇️
lua/neo-tree/sources/common/container.lua 67.55% <0.00%> (+0.52%) ⬆️
lua/neo-tree/ui/renderer.lua 70.95% <0.00%> (+0.90%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@musjj
Copy link
Contributor Author

musjj commented Dec 1, 2022

Alright, I figured out what's happening, it is some kind of race condition.
The error messages completely confused me, because neo-tree's test utilities mixed up the {expected} and {actual} arguments:

function mod.assert_buf_lines(bufnr, lines, linenr_start, linenr_end)
, which makes for a very misleading message.
But once I figured that out, the cause is obvious: there's a bit of a delay between sending the keys, and the folder tree actually expanding when git_status_async is true. I'm still not sure about the real culprit causing that delay, but putting in an vim.wait after sending the keys seem to fix the issue.

@cseickel
Copy link
Contributor

cseickel commented Dec 2, 2022

there's a bit of a delay between sending the keys, and the folder tree actually expanding when git_status_async is true. I'm still not sure about the real culprit causing that delay, but putting in an vim.wait after sending the keys seem to fix the issue.

That seems perfectly reasonable, if git status is async and the testing code is not, you would certainly expect the testing code to execute first.


I think this PR looks good, but I want to take the time to really read it carefully before merging and I haven't gotten that time yet. I'll do that tonight or this weekend.

Copy link
Contributor

@cseickel cseickel left a comment

Choose a reason for hiding this comment

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

Looks good, I just have one nit-pick comment.

Copy link
Contributor

@cseickel cseickel left a comment

Choose a reason for hiding this comment

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

This is good to go. Thanks!

@cseickel cseickel merged commit 2e8d1ba into nvim-neo-tree:main Dec 4, 2022
@musjj musjj deleted the async-git-ignore branch November 16, 2024 18:16
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.

2 participants