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

Use gitstatus in the vcs segment if it's available #2

Merged
merged 6 commits into from
Mar 6, 2019
Merged

Use gitstatus in the vcs segment if it's available #2

merged 6 commits into from
Mar 6, 2019

Conversation

romkatv
Copy link
Contributor

@romkatv romkatv commented Mar 5, 2019

This is an alternative to #1 based on your suggestions. It modifies the existing vcs segment to do the following:

if (gitstatus is available) {
  if (git repo) {
    use gitstatus
  } else {
    use vc_info with hg and svn backends enabled (without git)
  }
} else {
  use vc_info with git, hg and svn backends enabled
}

Because p10ks_vcs_async runs in a fork, gitstatus must be sourced before p10k. Basically, the process where p10ks_vcs_async runs must have environment variables that gitstatus plugin sets when it's loading. This is brittle and will certainly trip users. I don't know how to fix this.

The generated prompt isn't exactly identical to what vcs_info produces. vcs_info uses spaces before and between icons inconsistently. Take a look:

> master◌
> master ✚
> master ✚◌ 
> master ✚◌
> master ✚ ●◌

The first and the last prompt look weird. With gitstatus it's consistent:

> master ◌
> master ✚
> master ✚◌ 
> master ✚◌
> master ✚●◌

It's possible that I'm missing the intent of the apparent missing and extra spaces vcs_info generates. Let me know if so.

Another difference is that gitstatus prints remote branch name if it's present and is different from local. I personally find it useful. In most cases it doesn't use up space because there is usually either no remote branch or it's the same as local.

And lastly, gitstatus shows the number of stashes. For people who don't use stashes there will be no wasted space. For those who do, they'll appreciate the info.

These extra pieces of info come at no performance cost.

Here's an example (also in the comments):

feature→master ✚●◌ +1 -2 ⋄3

We are on branch "feature" that tracks "origin/master". We have staged, unstaged and untracked files. We are 1 commit ahead and 2 commits behind. We have 3 stashes.

I also cleaned up the existing code. I hope you don't mind.

To test on Docker:

docker run -e LANG=C.UTF-8 -e LC_ALL=C.UTF-8 -e TERM=$TERM -it --rm ubuntu bash -c '
  set -uex
  apt update
  apt install -y zsh git
  cd
  git clone https://github.com/romkatv/gitstatus.git
  git clone -b gitstatus2 https://github.com/romkatv/p10k
  echo "
    source ~/gitstatus/gitstatus.plugin.zsh
    source ~/p10k/presets/robobenklein.p10kp.zsh
    source ~/p10k/p10k.zsh" >~/.zshrc
  cd p10k
  zsh -i'

Remove source ~/gitstatus/gitstatus.plugin.zsh to check how it works when gitstatus isn't available.

Note that prompt is fast only in git repos (about 3 times faster than before). In directories that aren't repos at all (e.g., the root dir), the prompt is still slow because it calls vcs_info. Even though I disable git backend on this call, it doesn't help much. Many people don't care about non-git repos (myself included), so they shouldn't have to suffer. To solve this problem, I made vcs backends configurable. Everything will be fast if you add the following stanza to presets/robobenklein.p10kp.zsh:

typeset -gA p10ks_vcs
p10ks_vcs[backends]=git

P.S.

I haven't tested it with svn or hg. Please do test if you intend to merge this PR.

@romkatv romkatv mentioned this pull request Mar 5, 2019
@romkatv
Copy link
Contributor Author

romkatv commented Mar 6, 2019

I'm surprised you merged #1 rather than this PR. Your idea to extend the existing vcs segment instead of adding a dedicated gitstatus segment was a good one.

This PR is more flexible. It's safe, too.

  1. If a user doesn't have a working gitstatus, vcs behaves just like before. Note that gitstatus performs self-testing when loading and won't set its global parameters unless self-checks pass. So when vcs segment thinks there is gitstatus, there really is. (The self-test currently takes up to 5 seconds on systems where gitstatus doesn't actually work, but it's not something your code should care about because it's triggered by the user sourcing gitstatus and not by p10k. I'll reply to your How to check if binary is compatible? romkatv/gitstatus#4 with more details.)
  2. If a user does have gitstatus, it's used to pull info from git repos.
  3. If a user has configured backends other than git (by default svn and hg are enabled), then vcs_info is used but only when we know we aren't in a git repo.

With #1 users who care about non-git repos are out of luck. There is no way for them to benefit from gitstatus at all. If they enable only gitstatus segment, they won't get anything from svn and hg. If they enable both segments, they'll get duplicate segments in git repos and slow prompt. The only option is to enable vcs segment and not use gitstatus.

If you don't like the extra bits I added to the prompt (remote branch name and stashes), or the code cleanup I've performed, or configurable backends, I can remove those. I believe the overall approach of this PR is superior to #1 but I'm not attached to details.

@robobenklein
Copy link
Owner

I merged #1 so that I could make the feature available immediately while I realize how much work I want to redo on the vcs segment. I still plan on having gitstatus as an option in the default vcs segment, just that I need to rewrite much of the vcs segment to better support using different backends.

It's not that I don't like your changes, it's just that it made me realize how much I want to change. (In addition to renaming the whole proj. at some point.)

I'll probably merge this to a dev branch and keep iterating...

@robobenklein
Copy link
Owner

Right now I'm investigating if it's possible to load gitstatus async (zplugin wait) and still have the zsh-async worker pick up on it...

@robobenklein robobenklein changed the base branch from master to dev March 6, 2019 15:47
@robobenklein robobenklein merged commit 9e8ece9 into robobenklein:dev Mar 6, 2019
@romkatv
Copy link
Contributor Author

romkatv commented Mar 7, 2019

I'm afraid I broke this code with the latest gitstatus changes. To fix this, you'll need to do the following:

Once gitstatus is sourced, you must call gitstatus_start P10K where P10K is kind of like a worker name. This is to allow multiple plugins/themes to use gistatus in the same shell. You can call gitstatus_start P10K multiple times and it'll return instantly and without errors if worker has already been started. However, it's better to call this function only once to avoid repeatedly causing high latency if gitstatus cannot initialize for some reason.

Where querying gitstatus, you currently have this line:

(( ${backends[(I)git]} )) && [[ -v GITSTATUS_DAEMON_PID ]] && gitstatus_query_dir

It should become this:

(( ${backends[(I)git]} )) && [[ gitstatus_check P10K ]] && gitstatus_query -t 5 P10K && [[ $VCS_STATUS_RESULT=ok-sync ]]

Several things here:

  1. The default timeout is gone. You have to pass -t 5 to have the 5s timeout this code had before.
  2. The function name has changed.
  3. GITSTATUS_DAEMON_PID is gone but there is gitstatus_check P10K instead.
  4. gitstatus_query now returns success if the current directory isn't a git repo. This is to distinguish between genuine errors and "not-git" state. You have to examine $VCS_STATUS_RESULT to see whether there is git repo or not. The code can be restructured now, which is nice, but I'll leave it up to you.

Sorry about all these changes. They were necessary to add tricky async support in Powerlevel10k.

P.S.

Check out my latest improvements in Powerlevel10k. I'd love to know what you think of it. Give it a try on one of your slow machines.

@robobenklein
Copy link
Owner

I think these changes are pretty useful tbh (makes async use so much easier), thanks for the update!

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