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

Perform all git checks (vcs_info) asynchronously #273

Merged
merged 11 commits into from
Apr 27, 2017

Conversation

mafredri
Copy link
Collaborator

This is a quick experiment to perform vcs_info asynchronously. For anyone trying this out, would appreciate some feedback on how it feels / behaves / problems / etc.

Here we try to detect if the git toplevel has changed so that we can immediately clear the git branch / dirty / arrow status. The problem is, git rev-parse --show-toplevel (as used by vcs_info) might differ from $PWD (e.g. because of symlinks).

To mitigate the above problem to some extent we store the current $PWD when entering a git project, this $PWD is then used in future comparisons to see if the new $PWD is at least a partial match of $PWD, if not everything is cleared immediately.

To illustrate what happens:

  1. cd ~/pure
  2. prompt_pure_async_vcs_info detects a new git top level /home/user/pure, prompt_pure_vcs_info[pwd]=/home/user/pure
  3. cd arch
  4. [[ /home/user/pure/arch =~ ^/home/user/pure ]] is true, meaning we did not likely change git toplevel, keep the branch / arrow / dirty status

And in the case of first entering a sub folder:

  1. cd ~/pure/arch
  2. prompt_pure_async_vcs_info detects a new git top level /home/user/pure, prompt_pure_vcs_info[pwd]=/home/user/pure/arch
  3. cd ..
  4. [[ /home/user/pure =~ ^/home/user/pure/arch ]] is not true, so git info is cleared
  5. prompt_pure_async_vcs_info returns and restores the git info, this time setting prompt_pure_vcs_info[pwd]=/home/user/pure

@mafredri mafredri changed the title Perform vcs_info asynchronously (experimental) [WIP] Perform vcs_info asynchronously (experimental) Jan 17, 2017
@sindresorhus
Copy link
Owner

sindresorhus commented Jan 28, 2017

I've been running this for a week now and haven't noticed any issues, but can't say I've noticed any speed improvement either, although my computer is pretty fast.

Would be good if someone with a slower computer could try this out.

@sindresorhus
Copy link
Owner

@pmbenjamin @seemethere @peterkc @jmagnusson Could you test this out?

@mafredri
Copy link
Collaborator Author

Thanks for giving it a good spin! I'd say the biggest improvement in prompt responsiveness can see seen when issuing consecutive commands or when the system is otherwise under heavy load.

I've noticed one issue, when I rebased a branch without pushing the changes, rebranched it (checkout -b) so that there was no origin, the git arrows were not reset. I'll look to fix this soon.

Some other cases where I think this could provide better responsiveness is on spinning disks, and partitions mounted over the network (esp. sshfs) although I don't have any setups like this.

@seemethere
Copy link
Contributor

seemethere commented Jan 28, 2017

I'll give it a go @sindresorhus. I'll give it a couple of days and report back.

@sindresorhus
Copy link
Owner

@mafredri I discovered a bug. It fails when you try to enter a git repo where the directory name contains spaces:

~/Downloads
❯ cd 'foo bar'

~/Downloads/foo bar
❯
prompt_pure_async_callback:9: bad set of key/value pairs for associative array
❯

@mafredri
Copy link
Collaborator Author

Good catch @sindresorhus! Should be fixed in d8de8f6.

@seemethere
Copy link
Contributor

Can't really say I've had any issues with this branch so far.

@mafredri
Copy link
Collaborator Author

mafredri commented Feb 10, 2017

Thanks for testing @seemethere! Did you notice that it behaves differently in any way? Does it feel more responsive / sluggish?

The biggest difference here is that the branch name (when changing branches) is updated after the prompt is rendered, meaning e.g. when you switch from master -> development, the next prompt will include master, which then almost immediately gets overwritten with a new prompt containing development. I imagine some people will notice this more than others.

I have an idea for improving it though, we could rewrite the algorithm in prompt_pure_preprompt_render to allow us to patch the prompt instead of a full update like we currently do. I believe this should speed up the branch name update but it's not a given, of course.

PS. I pushed a fix for the git arrows remaining after switching to a branch with no upstream.

@mafredri
Copy link
Collaborator Author

mafredri commented Feb 11, 2017

Ok, so I just pushed a commit for performing partial prompt updates when possible. To me it feels slightly snappier, can you notice a difference (e.g. when switching branches)?

It could be further optimized by keeping track of the cursor position using zle widgets. This would allow us to optimize our use of ansi-escape codes to move the smallest amount of distance necessary to perform the update. Keeping track of the position would be a precursor for fixing #143 as well.

Not to worry 26f2266 is pretty big, but a large chunk of that is updated comments. The logic change is still pretty small.

EDIT: Nevermind, I'm smoking crack... it's not working 😅.

@mafredri
Copy link
Collaborator Author

mafredri commented Feb 11, 2017

I just amended my previous commit with c67e20e. Now the partial preprompt update is actually be working. Small caveat: does not work on Emacs ansi-term currently due to the use of ESC[nG, maybe others. See commit for more details

EDIT: Force pushed again to fix a typo in the commit message -_-'' (commit ref above updated)

@mafredri
Copy link
Collaborator Author

@Hotschke, @sevanteri: are you guys still using pure with sshfs? I'd be happy if you could give this branch a spin, without disabling vcs_info on sshfs mounts :)!

@mafredri
Copy link
Collaborator Author

Whoa, turns out we should be able to greatly simplify the preprompt update method... mafredri@24e3a3a. I remember trying out newlines in the prompt before, but I couldn't get it working. Dunno what to say, seems to work better than the ansi escape fiddling.

@mafredri
Copy link
Collaborator Author

Rebased onto latest master.

@Hilzu
Copy link

Hilzu commented Feb 17, 2017

@sindresorhus asked me to try this so I did. Have to say that it feels worse than before. I can't detect any latency when switching repositories without this so I can't really see any speed benefits.

Now when changing to a different repository with unpulled changes the down arrow appears for a while and then disappears after a second. This seems like a bug.

@mafredri
Copy link
Collaborator Author

Have to say that it feels worse than before. I can't detect any latency when switching repositories without this so I can't really see any speed benefits.

Thanks for the honest feedback. If you can explain, I would love to hear what exactly makes it feel worse.

This is indeed what worries me most with this PR. Pure is already so fast that on most machines the time it takes to check the git info is not easily noticeable to the human eye.

Now when changing to a different repository with unpulled changes the down arrow appears for a while and then disappears after a second. This seems like a bug.

Weird, have not seen this. Would you mind providing e.g. an asciinema recording?


Since vcs_info does a bit of unnecessary work, an alternative approach to this PR would be to stop using it in favor of executing the commands manually (e.g. to check if in git repo and the branch/status).

@Hilzu
Copy link

Hilzu commented Feb 17, 2017

My feeling about it being worse was completely based on the fiddly pull arrow. Now I'm unable to reproduce the issue anymore with this branch ¯\_(ツ)_/¯

@mafredri
Copy link
Collaborator Author

Thanks @Hilzu for clarifying and trying to reproduce. I think I found the reason you encountered the issue. An edge case where a git fetch fails (for whatever reason) and is interpreted as a failed check for changes to upstream. Should be fixed in 5ebb0e2.

This commit adds a third mode to the preprompt update: number of lines
between previous and current preprompt are equal. We then compare the
preprompts, part-by-part, to find the first part that differs. From this
we can calculate the offset of the update.

Since "%~" is the only prompt substitution we use in the preprompt, we
now expand it in-place so that we get comparable preprompts from the
get-go.

The preprompt was also turned into an array (preprompt_parts) to allow
comparing the different parts in the prompt.

NOTE: We previously used `ESC[${COLUMNS}D` to make sure the cursor is at
the beginning of the line. Turns out this does not work as expected with
prompts that occupy multiple lines due to small terminal size or long
paths. The thought was that it would stop moving the cursor when the
first column of the line was reached, however, it seems it can continue
on to the following line.

For the above reason, we no use `ESC[1G` to move the cursor into the
first column. This is not part of ANSI.SYS, however, and might not be
supported by all terminal emulators. Works in Terminal, iTerm, Hyper.
Does not work in Emacs ansi-term.

One way to fix this is to fetch the current cursor position from ZLE
using a ZLE widget. If we know the starting position, we can calculate
the rest.
Since vcs_info is running inside the async task, configuration changes
to vcs_info have no effect after the async worker has started. We might
as well make this more explicit and allow the user to use vcs_info as
they see fit.
This method allows us to remove all the custom update logic using
ansi escape sequences and greatly simplifies the preprompt render
function.
@mafredri
Copy link
Collaborator Author

I've been noticing some visual glitches with this branch whenever I type before the prompt is ready, I don't notice the same with 9496e07 so I decided to merge it in (and rebase on master).

The preprompt must not consist of local variables, this would prevent
zle reset-prompt from behaving correctly in all situations.
@sindresorhus
Copy link
Owner

I've been running this for a couple of days now and seems to work fine.

I'd say let's merge this now. Mainly for 9496e07.

@mafredri
Copy link
Collaborator Author

Thanks for testing, and I agree. The simplified prompt update logic should even fix some old issues that are still open.

The git-parent-folder detection could surely still be improved, but the benefit would be minimal. Moving for merge now.

@mafredri mafredri changed the title [WIP] Perform vcs_info asynchronously (experimental) Perform all git checks (vcs_info) asynchronously Apr 27, 2017
@mafredri mafredri merged commit 4cdd0cf into sindresorhus:master Apr 27, 2017
@forivall
Copy link
Contributor

forivall commented May 3, 2017

yay, i got my fork working with this change master...forivall:underline-repo-name

@mafredri
Copy link
Collaborator Author

mafredri commented May 5, 2017

Nice @forivall, glad you were able to patch it in 😄!

@mafredri mafredri deleted the async-everything branch May 5, 2017 03:28
@lugoues
Copy link

lugoues commented Jun 11, 2017

Has any thought been given on how to bring back the option allowing users to use vcs_info to customize the prompt?

@mafredri
Copy link
Collaborator Author

@lugoues not really, changing Pure through vcs_info configuration was never really a feature, just a side-effect of using it. What changes would you make?

@lugoues
Copy link

lugoues commented Jun 14, 2017

@mafredri Just various color tweaks and bold/italics based on branch or local repo state. Nothing too special. I didn't realize that using vcs_info wasn't really a feature Pure supported though, so no worries.

@forivall
Copy link
Contributor

@lugoues Yeah, the recommended solution for customizations is to maintain your own fork, which, unfortunately for those of us who wants customization, shifts the burden of maintainership to us rather than the project maintainers. But they've often been willing to help with solving problems on customizations (as long as you don't ask them to maintain it 😛).

This is what I do for my fork where I make the name of the repo bold: https://github.com/forivall/pure/tree/underline-repo-name

(speaking of which, I need to submit a PR upstream for just this commit forivall@2848afb )

@mafredri
Copy link
Collaborator Author

@lugoues there is one way to do that still, without maintaining your own fork, but it gets a bit hairy...

# Rename the prompt_pure_preprompt_render function to prompt_pure_preprompt_render_orig.
typeset prompt_pure_preprompt_render_def=$(whence -f prompt_pure_preprompt_render)
eval ${prompt_pure_preprompt_render_def/prompt_pure_preprompt_render/prompt_pure_preprompt_render_orig}

# Hijack the pure renderer function.
prompt_pure_preprompt_render() {
	local cur_branch=$prompt_pure_vcs_info[branch]

	# If branch is set and we haven't already modified it...
	if [[ -n $prompt_pure_vcs_info[branch] ]] && [[ $cur_branch[1] != '%' ]]; then
		# Set branch color to yellow.
		prompt_pure_vcs_info[branch]="%F{2}${cur_branch}%f"
	fi

	# Call the original renderer.
	prompt_pure_preprompt_render_orig $@
}

You could drop this code anywhere after prompt pure and that would change the current git branch to yellow, as an example.

Takes advantage of the fact that the renderer is called anytime the prompt is updated, so we can do some preprocessing. Now, this probably goes without saying, but this could break at any time, so be warned 😅.

@lugoues
Copy link

lugoues commented Jun 14, 2017

@mafredri What are your thoughts on changing prompt_pure_async_vcs_info to have a call out to a custom function that a user, if they were so inclined, could create to modify their zstyle?

Something like this?

customize_vcs_info() {
  zstyle ':vcs_info:git*' formats " `tput sitm`%b`tput ritm`" "x%R"
}
prompt_pure_async_vcs_info() {
	setopt localoptions noshwordsplit
	builtin cd -q $1 2>/dev/null


	# configure vcs_info inside async task, this frees up vcs_info
	# to be used or configured as the user pleases.
	zstyle ':vcs_info:*' enable git
	zstyle ':vcs_info:*' use-simple true
	# only export two msg variables from vcs_info
	zstyle ':vcs_info:*' max-exports 2
	# export branch (%b) and git toplevel (%R)
	zstyle ':vcs_info:git*' formats '%b' '%R'
	zstyle ':vcs_info:git*' actionformats '%b|%a' '%R'

	whence -w customize_vcs_info > /dev/null && customize_vcs_info

	vcs_info

	local -A info
	info[top]=$vcs_info_msg_1_
	info[branch]=$vcs_info_msg_0_

	print -r - ${(@kvq)info}
}

@mafredri
Copy link
Collaborator Author

@lugoues while I do want to make Pure more easily hackable, I dislike that style of customization. I would see it as an intentional feature and it would have to be documented and maintained.

Furthermore, since this is an async task there is a timing aspect to it as well. The called function would have to be defined by the time the pure async worker is started, otherwise it will be unreachable. The user also can't rely on global shell state (e.g. setting a variable later) to change behavior due to this fact. It would be hard to properly document this behavior.

The easiest approach would simply be to copy-paste the entire prompt_pure_async_vcs_info function right after prompt pure and do the modifications you want (it would replace the one defined in Pure).

We could also split part of it into a separate function which you could override, but this gives very little value IMO. What I mean is something like this:

prompt_pure_vcs_info() {
	# configure vcs_info inside async task, this frees up vcs_info
	# to be used or configured as the user pleases.
	zstyle ':vcs_info:*' enable git
	zstyle ':vcs_info:*' use-simple true
	# only export two msg variables from vcs_info
	zstyle ':vcs_info:*' max-exports 2
	# export branch (%b) and git toplevel (%R)
	zstyle ':vcs_info:git*' formats '%b' '%R'
	zstyle ':vcs_info:git*' actionformats '%b|%a' '%R'

	vcs_info
}

prompt_pure_async_vcs_info() {
	setopt localoptions noshwordsplit
	builtin cd -q $1 2>/dev/null

	prompt_pure_vcs_info

	local -A info
	info[top]=$vcs_info_msg_1_
	info[branch]=$vcs_info_msg_0_

	print -r - ${(@kvq)info}
}

But the problem persist that if you make any kind of changes to Pure's vcs_info configuration, it could break in any future update of Pure and result in weird bugs. In this sense I believe it's better to be fully aware of what you're doing (e.g. by being forced to replace entire functions, etc.).

@lugoues
Copy link

lugoues commented Jun 18, 2017

@mafredri I see, I guess that makes some sense. I did notice in #306 there is talk of adding color support using zstyle, could this approach be taken to setup custom vcs_info settings too? Presumably the same issues apply there as well, no?

kutsan pushed a commit to kutsan/pure that referenced this pull request Jun 19, 2023
* Perform vcs_info checks asynchronously

* Configure vcs_info inside async task

Since vcs_info is running inside the async task, configuration changes
to vcs_info have no effect after the async worker has started. We might
as well make this more explicit and allow the user to use vcs_info as
they see fit.

* Rely on zle reset-prompt to update prompt

This method allows us to remove all the custom update logic using
ansi escape sequences and greatly simplifies the preprompt render
function.

* Clear git arrows when there is no upstream or git fetch fails
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

6 participants