Skip to content

feat(git): async git interface for get_repository_root #637

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 2 commits into from
Dec 8, 2022
Merged

feat(git): async git interface for get_repository_root #637

merged 2 commits into from
Dec 8, 2022

Conversation

musjj
Copy link
Contributor

@musjj musjj commented Dec 5, 2022

This adds a callback argument to git.utils.get_repository_root, which allows the function to be called asynchronously.
Callers are also adjusted so that it respects the async_git_status option.

Also, I've been wondering, is it really necessary to have synchronous support at all? As long as the asynchronous version is implemented correctly, I can't imagine a user that would prefer to use the synchronous version.

Once the async paths are battle-tested and its rough edges polished, what do you think about dropping non-async support in the next major version? I feel that it would simplify the codebase.

@codecov
Copy link

codecov bot commented Dec 5, 2022

Codecov Report

Merging #637 (e0b543a) into main (2e8d1ba) will decrease coverage by 0.24%.
The diff coverage is 45.85%.

@@            Coverage Diff             @@
##             main     #637      +/-   ##
==========================================
- Coverage   50.50%   50.26%   -0.25%     
==========================================
  Files          47       47              
  Lines        6187     6199      +12     
==========================================
- Hits         3125     3116       -9     
- Misses       3062     3083      +21     
Impacted Files Coverage Δ
lua/neo-tree/setup/deprecations.lua 40.81% <0.00%> (-1.74%) ⬇️
...eo-tree/sources/filesystem/lib/filter_external.lua 7.40% <0.00%> (ø)
lua/neo-tree/sources/filesystem/lib/fs_scan.lua 71.49% <0.00%> (ø)
lua/neo-tree/sources/filesystem/lib/fs_watch.lua 14.11% <0.00%> (-2.33%) ⬇️
lua/neo-tree/ui/selector.lua 5.10% <0.00%> (ø)
lua/neo-tree/git/utils.lua 66.66% <60.00%> (-27.09%) ⬇️
lua/neo-tree/git/status.lua 56.41% <74.28%> (+0.91%) ⬆️
lua/neo-tree/log.lua 40.69% <100.00%> (-2.00%) ⬇️
lua/neo-tree/ui/highlights.lua 93.51% <100.00%> (+0.24%) ⬆️
lua/neo-tree/ui/renderer.lua 70.55% <100.00%> (-0.41%) ⬇️
... and 2 more

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

@cseickel
Copy link
Contributor

cseickel commented Dec 5, 2022

Also, I've been wondering, is it really necessary to have synchronous support at all? As long as the asynchronous version is implemented correctly, I can't imagine a user that would prefer to use the synchronous version.

Believe it or not, some people do prefer sync over async. The reason is that async does effectively make the whole thing slower. Also, some people notice and are really bothered by the flicker that async introduces. What we settled on is that by default, Neo-tree will use sync methods when opening for the first time for that immediate response, then switch to async for any background refreshes.

I think git status is the only thing that is always async by default now, but the git ignored lookup needs to happen before render so that definitely needs both the sync and async code paths.

@musjj
Copy link
Contributor Author

musjj commented Dec 6, 2022

the flicker that async introduces

I've experienced this too, but it turns out that the flicker is caused by the non-async git calls. With the previous PR and this one, the flicker is now completely gone (at least with my config). As the overall speed, I'm not really sure since measuring it seems tricky.
But I guess you're right, non-async options should be kept for now.

Btw, I'm not so sure what's going on with stylua-check, I've ran the formatter in my editor, so I'm not sure what it's complaining about.

@cseickel
Copy link
Contributor

cseickel commented Dec 6, 2022

Btw, I'm not so sure what's going on with stylua-check, I've ran the formatter in my editor, so I'm not sure what it's complaining about.

The problem is the stylua 14.x is incompatible with the behavior of 13.x. When the switch happened, I stuck with 13.x for a while while distributions were being updated with the new version. I think everyone is updated to 14.x now, but this project still needs to be cut over. Just one of a million little things I don't have time to do.

#422

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