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

Git States #337

Closed
wants to merge 3 commits into from
Closed

Git States #337

wants to merge 3 commits into from

Conversation

BrandonRoehl
Copy link

screen shot 2017-08-03 at 11 56 05 am

4 major changes

  1. Speed improvement with the prompt_pure_preprompt_render not passing expressions to the PROMPT
  2. Config options for the prompt arrow removed because you can set PROMPT anyways
  3. Config options for colors in the top added. Why? Because 256bit colors are nice for not changing the terminal setup. And forcing them kinda ousts people without 256bit support in zsh or tmux.
  4. Git now shows the modified, untraced, and staged files denoted by a * . + respectively
  5. Config PURE_GIT_UNTRACKED_DIRTY removed in favor of displaying the .

My Config for 256bit colors

export PURE_PATH_COLOR=045
export PURE_GIT_ARROW_COLOR=87
export PURE_GIT_COLOR=245
export PURE_TIME_COLOR=215
PROMPT='%(12V.%F{242}%12v%f .)%(?.%F{177}.%F{203})❯%f '

@BrandonRoehl BrandonRoehl changed the title Original master Git States Aug 3, 2017
@BrandonRoehl
Copy link
Author

The * . + symbols could be something completely different and in a different order too. This is just what I came up with

@BrandonRoehl BrandonRoehl deleted the original-master branch August 3, 2017 23:15
@BrandonRoehl BrandonRoehl restored the original-master branch August 4, 2017 16:14
@BrandonRoehl BrandonRoehl reopened this Aug 4, 2017
@mafredri
Copy link
Collaborator

Hi, and thanks for the PR.

I would suggest splitting this up in separate PR's so each topic of change can be discussed in separation.

Here are my initial thoughts:

  1. Speed improvement with the prompt_pure_preprompt_render not passing expressions to the PROMPT

How much of a speed improvement is this, did you perform any benchmarks?

Also, I believe this change might open us up to a security hole that can e.g. be triggered by njhartwell/pw3nage, but I haven't tested the PR. Can you confirm if this is an issue or not?

  1. Config options for the prompt arrow removed because you can set PROMPT anyways

This is a backwards-incompatible change, anyone relying on the environment variable would be affected. Although setting the prompt manually is possible, it's not very intuitive or obvious. It might even break in future, or new features might be missing.

  1. Config options for colors in the top added. Why? Because 256bit colors are nice for not changing the terminal setup. And forcing them kinda ousts people without 256bit support in zsh or tmux.

Colors is something we're actively considering, however, we would like to avoid introducing new environment variables. This is currently being discussed in #256 and #306 (also #292). It would be great if we could see a PR for a zstyle approach for colors 😄.

  1. Git now shows the modified, untraced, and staged files denoted by a * . + respectively

I'm not sure we want to go down this path with Pure, personally I don't see much value in showing that something is staged or untracked since an indicator is not granular enough. I use git status for that. But this would be better discussed in separation.

  1. Config PURE_GIT_UNTRACKED_DIRTY removed in favor of displaying the .

Hmm, I think the reason this option exists (IIRC) is to speed up dirty checking for large repos (think chromium). Can you confirm @sindresorhus?


PS. We use tab-indentation instead of spaces in Pure, I would recommend installing the editorconfig plugin for your editor for future PR's 🙂.

@sindresorhus
Copy link
Owner

Hmm, I think the reason this option exists (IIRC) is to speed up dirty checking for large repos (think chromium). Can you confirm @sindresorhus?

Yup

@sindresorhus
Copy link
Owner

And I agree with everything @mafredri said.

@BrandonRoehl
Copy link
Author

Also, I believe this change might open us up to a security hole that can e.g. be triggered by njhartwell/pw3nage, but I haven't tested the PR. Can you confirm if this is an issue or not?

This is talking about examples where the PROMPT has the evaluating such as what you had

PROMPT='${BRANCH}'
echo $PROMPT # > ${BRANCH}
# Evaluates BRANCH in every dir so if BRANCH has a resolvable path to a file can be executed

what I suggested

PROMPT="${BRANCH}"
echo $PROMPT # > master
# even after dir change
# This is evaluated in our environment so unless the script we have has a function that is resolvable nothing happens

Originally

Note every command to get evaluated takes time as well
screen shot 2017-08-12 at 11 41 44 am

After

Only going to evaluate colors no $
screen shot 2017-08-12 at 11 39 18 am

Tested that it still works because you were setting the stuff anyways it was just changing from single quotes to double quotes. This is the really minor speed improvement because the prompt doesn't evaluate anything for values. The prompt is slower than in the pre command because you are already doing variable and register access.

The general idea is you should never set PROMPT in the pre command but if your doing it anyways why not.
The best speed is just set PROMPT once in setup and use the psvar[] to handle the few substitutions in it removing all $ signs from your prompt and security issues.

@BrandonRoehl
Copy link
Author

I am working on a rewrite BrandonRoehl/zsh-clean

  1. It sets PROMPT once
  2. psvar[] is used
  3. Because of this the PROMPT is 100% customizable after
  4. It doesn't require external libraries such as async this is handled by zsh &
  5. The precmd is 4 lines long
  6. vcs_info is configured to use hooks allowing it to support ALL of the vcs that vcs_info allows while still supporting git unstaged and git arrows as an action format

The precmd is slightly slower because it lacks callbacks from async.

@mafredri
Copy link
Collaborator

mafredri commented Aug 12, 2017

I didn't get a clear answer regarding pw3nage, so I tried it out myself and this PR is indeed vulnerable to it. Expanding the git branch name in-place, as in this PR, is not safe.

Original: '$BRANCH' -> render prompt -> prints: $(./pw3n).

PR: "$BRANCH" -> "$(./pw3n)" -> render prompt -> evaluates $(./pw3n) (e.g. runs the command).

The prompt is slower than in the pre command because you are already doing variable and register access.

Do you have any facts to back this up? I would think that variable lookups are so cheap that it's not worth thinking about (at this scale).

@mafredri
Copy link
Collaborator

The general idea is you should never set PROMPT in the pre command but if your doing it anyways why not.

I've never heard of this, could you point me to some resource or provide background?

The best speed is just set PROMPT once in setup and use the psvar[] to handle the few substitutions in it removing all $ signs from your prompt and security issues.

I'd also be interested to learn more of this, e.g. why would it be faster to use psvar? Do you know any resources?

@BrandonRoehl
Copy link
Author

Do you have any facts to back this up? I would think that variable lookups are so cheap that it's not worth thinking about (at this scale).
Seeing that the pre render then allows that sting to be evaluated after I would agree on the fact that that the retrieval from memory would probably be worth it to keep it from rendering.

I'd also be interested to learn more of this, e.g. why would it be faster to use psvar? Do you know any resources?

psvar isn't inherently any faster that doing a standard lookup of any other variables. What it offers is
%v# properly escaping and not evaluating anything. Setting the prompt once is what speeds it up a lot. Because then the big o is what decreases when you do less assignments and reads

https://www.zsh.org/mla/users/2005/msg00863.html

Big O

  1. check cmd time
  2. unset timestamp
  3. VCS_INFO (async)
  4. set title
  5. unset ps var 12
  6. test for virtual environment
  7. store virtual environment

pre render
8 test
6 assignments
+= is and assignment and lookup and you have 4 of them
and then joining ps1 into prompt every single time

This is the pre command trace

Where as if you assign prompt only once then all you have are variable lookups

it could be just

  1. check cmd time
  2. unset timestamp
  3. VCS_INFO (async)
  4. set title
  5. assign psvar

psvar

psvar can also test the existence of variables in prompt its own way too using %(V1. %v1.) to add a space before the v1

@mafredri
Copy link
Collaborator

I'm not unfamiliar with psvar, I know what it does, but I can't find anything regarding prompt rendering speed in zsh-users 9188 that you linked to. I would be very interested in some facts regarding prompt rendering speed when evaluating variables versus not (e.g. using psvar).

PS. I don't see how Big-O has anything to do with this. We're not really expressing the complexity of algorithms here, just a few assignments ;-).

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