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

Refactor async logic and allow for async git status. #108

Closed
wants to merge 27 commits into from

Conversation

mafredri
Copy link
Collaborator

This pull request replaces my previous PR (#106) and fixes issues #81 and #102.

Discussions welcome.

  • Pure now uses a background worker for git fetch and git status
  • Only one git fetch/status can run at a time
  • All worker jobs are eliminated on directory change (consider passing
    only git working tree?)
  • If a git status takes longer than 2 second, rely on cached result
    for 30 minutes (1800 seconds)
  • Disowned process no longer takes care of rendering the git up/down
    arrows. It is done by the parent process
  • Entire preprompt can be redrawn to allow for updating git status,
    etc. Works well in both smaller and larger terminals.
  • Arrows can be drawn immediately if the data is available

Some ideas:

  • Extend the periodical check to perform new checks even when no new action has been take on the terminal. So even a background terminal could inform you that there are changes to pull, etc.
  • Make git status execution time-limit configurable (instead of hard-coded 2 seconds)
  • Now worker checks are performed 1s after execution and periodically every 15 seconds. Maybe check for worker results more often until children have performed results. Could also increase periodic time to e.g. 1min and make it configurable.

@mafredri
Copy link
Collaborator Author

mafredri commented Mar 2, 2015

I made my async implementation into a library: https://github.com/mafredri/zsh-async

I felt that I cluttered down your theme pretty radically with all my code changes. Thoughts about importing a "library" instead?

@sindresorhus
Copy link
Owner

Awesome! :)

consider passing only git working tree?

Yes

Extend the periodical check to perform new checks even when no new action has been take on the terminal. So even a background terminal could inform you that there are changes to pull, etc.

How would that work? If there were any changes, would my prompt suddenly get the arrow interactively?

Make git status execution time-limit configurable (instead of hard-coded 2 seconds)

I'd prefer not to unless there's a good reason.

If a git status takes longer than 2 second, rely on cached result for 30 minutes (1800 seconds)

Maybe we should use a separate color on the mark so the user is aware?

Now worker checks are performed 1s after execution and periodically every 15 seconds. Maybe check for worker results more often until children have performed results. Could also increase periodic time to e.g. 1min and make it configurable.

Can you elaborate, is the "worker checks" doing "git fetch" every 15 seconds or do you mean check for result from the worker?

I felt that I cluttered down your theme pretty radically with all my code changes. Thoughts about importing a "library" instead?

I would love to use a lib like yours instead. How would we do that though?

@mafredri
Copy link
Collaborator Author

consider passing only git working tree?

Yes

Agreed, this is the best course of action here, no point in stopping a process unless we are moving out of the git working tree. Will fix.

How would that work? If there were any changes, would my prompt suddenly get the arrow interactively?

Precisely! Say you have two open terminals for the same git directory, both old, perform git fetch on one (assuming changes locally / upstream) the other terminal would be updated with arrows during the next periodical check.

Currently the periodic check is run every 15 minutes, but it only checks for the tasks run by that terminal. This could go two ways (although I'm not too sure how I feel about the terminal performing tasks in the background without intervention):

  1. Only run periodic check (e.g. every second) when there are tasks active, then stop
  2. Combine 1. and run additional periodic check e.g. every 1 minute which could also launch new git status / check for local / remote commits

I'd prefer not to unless there's a good reason.

I can appreciate that, it was just a thought since 2 seconds was something I kind of pulled out of the hat. Felt that maybe 1 second is being a bit too aggressive (i.e. not that stressful) and more than 2 then there might be an actual cost somewhere.

Maybe we should use a separate color on the mark so the user is aware?

That's a great idea! I was only thinking about possibly indicating that git status is running, and indicating that it's "cached" hadn't crossed my mind :)

Can you elaborate, is the "worker checks" doing "git fetch" every 15 seconds or do you mean check for result from the worker?

Yes, the worker check is only a check for results from a task that has been launched earlier (e.g. git status, git fetch). So if a task has not been launched by other means, the worker check will return empty handed. I guess this point kind of intersected with the earlier one about periodic checks, hope I brought some clarity to the table.

I would love to use a lib like yours instead. How would we do that though?

Are you asking about how we would include my lib in this project? If so, I'm not quite sure. I guess if I pushed it to npm, then that would allow pure to require it, but then that would not allow for easy independent installation. Also, having it as a git submodule would be one possibility, but that requires the additional submodule pulling / etc. Third option I can think of would be maybe to include it in this project as a separate file, updates would need to be imported retroactively.

Got any other ideas? In my lib I've already fixed some potential problems which exist in this PRs implementation, e.g. race-conditions for tasks that finish nearly at the same time, etc. So would be nice to rely on it instead of a custom one here :).

@sindresorhus
Copy link
Owner

Thanks for clarifications. Sounds good to me.

Third option I can think of would be maybe to include it in this project as a separate file, updates would need to be imported retroactively.

Let's go with this.

@mafredri
Copy link
Collaborator Author

Alright, that's great!

I've done most of the refactor for this already, but it still needs some fixing here and there. One thing I'm still trying to work out is something that would allow to get rid of the polling (scheduling) and that is sending a kill signal to the parent process. It works beautifully, and feels much more interactive than polling, but there's an edge case ZSH completely freezes if you send multiple kill signals at the same time / during user interaction. (I.e. holding enter in the terminal can eventually crash it). I'm thinking of rate-limiting kill-signals on the async-library side, but it's taking some time to crack :)...

@sindresorhus
Copy link
Owner

Oh, yeah, some kind of signal would be much better than polling. Awesome work on this btw :)

@mafredri
Copy link
Collaborator Author

Thanks, it's been a very painful process :)! Obviously something zsh wasn't meant to do :D..

I think I finally found a solution, or workaround rather, to the kill signal leading to zsh freezing up. I'm now sending a SIGWINCH (Window size change) which does not, for some magical reason, seem to be plagued by the lockups.

I'm not fully satisfied with how the async library needs to be enabled at the moment (updated the readme also) but it's the only way I can think of, thoughts?

- Pure now uses a background worker for git fetch and git status
- Only one git fetch/status can run at a time
- All worker jobs are eliminated on directory change (consider passing
only git working tree?)
- If a git status takes longer than 2 second, rely on cached result
for 30 minutes (1800 seconds)
- Disowned process no longer takes care of rendering the git up/down
arrows. It is done by the parent process
- Entire preprompt can be redrawn to allow for updating git status,
etc. Works well in both smaller and larger terminals.
- Arrows can be drawn immediately if the data is available
Without artificially delaying the git status, the kill-signal might
be received while precmd is still running, in which case the prompt
might not be re-rendered, but the new status from git will not make it
to the initial rendering of the preprompt.
@mafredri
Copy link
Collaborator Author

Looks like me rebasing onto master moved the commits to the end, sorry about that.

Anyway, with my last two commits I thought it might be smart (since it's now possible) to move the pull/push arrows beside the git branch. Thoughts?

New arrow placement

@mafredri
Copy link
Collaborator Author

With my last commits, I believe this branch is quite ready. I've been using it as my daily driver since the beginning, so most of issues should've been ironed out. There is an indication of "cached" git status now as well.

Cached git status

@sindresorhus
Copy link
Owner

This looks incredible! I just need to get some time to read through the code and some days to have it running on my own computer.

@sindresorhus
Copy link
Owner

Ok, finally had time to extensively try this out. Sorry about the long delay... It works perfectly! :) It might be a placebo effect, but I feel the normal prompt is faster now too. Super nice work @mafredri.

@mafredri Can you do one final rebase to fix the merge conflict?

@mafredri
Copy link
Collaborator Author

Glad you liked it @sindresorhus! I've updated the PR, but If you want I could still squash the commits together so that it doesn't pollute the git history quite as much.

@arthurvr
Copy link

I've been using this for a week or so and it's awesome 👍 Glorious work!

@mafredri mafredri closed this in 94ccd58 May 23, 2015
@sindresorhus
Copy link
Owner

Landed! :)

@sindresorhus
Copy link
Owner

@mafredri Would you happen to be interested in being the maintainer of pure? You've done an excellent job with this PR and you seem a lot more adept at shell scripting than me. No worries if not though.

@mafredri
Copy link
Collaborator Author

Thanks for the compliments @sindresorhus! I've never really taken over as a maintainer before, but sure, I'd be interested if you're looking to take some load of your hands :).

@sindresorhus
Copy link
Owner

Cool. It's not hard, and you're already pretty much doing it. Just triaging issues, reviewing PRs and looking for ways to improve the project :)

@mafredri
Copy link
Collaborator Author

Alright, sounds good 👍 !

@mafredri mafredri deleted the async branch November 27, 2015 18:33
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.

None yet

3 participants