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

[Bugfix] Fix errors emitted by VCS segment's untracked file check when in a submodule #1071

Merged
merged 6 commits into from Nov 16, 2018

Conversation

Projects
3 participants
@macserv

macserv commented Nov 16, 2018

  • Unify two "default values" sections to be consistent with other segments, and resolve double-set P9K_VCS_SHOW_SUBMODULE_DIRTY default in favor of the current documentation.
  • Fix fatal errors emitted when the CWD is inside a submodule's tree.
  • Modify logic so that the VCS_WORKDIR_HALF_DIRTY status reflects the untracked file state from the current repo down instead of always going back to the highest-level repo.
  • Modify flow to skip the check completely if the CWD is the local .git folder (it's not considered to be a part of the working tree).
  • Optimize the routine a bit by skipping the recursive submodule check if the VCS_WORKDIR_HALF_DIRTY status can be established by the presence of untracked files in the current repo's tree.

Matthew Judy added some commits Nov 16, 2018

Matthew Judy
Unify two "default values" sections to be consistent with other segme…
…nts, and resolve double-set `P9K_VCS_SHOW_SUBMODULE_DIRTY` default in favor of the current documentation.
Matthew Judy
Fix fatal errors emitted when the CWD is inside a submodule's tree. M…
…odify logic so that the "HALF_DIRTY" status reflects the untracked file state from the *current repo down* instead of always going back to the highest-level repo. Modify flow to skip the check completely if the CWD is the local `.git` folder (it's not considered to be a part of the working tree). Optimize the routine a bit by skipping the recursive submodule check if the HALF_DIRTY status can be established by the presence of untracked files in the current repo's tree.
@dritter

This comment has been minimized.

Collaborator

dritter commented Nov 16, 2018

Thanks for the quick fix @macserv . I'll have a deeper look later on.

Could you add a test for that?

Timings (see #1046 (comment)) are:

time (repeat 10; do; prompt_vcs left 1 false >/dev/null; done;)

Scenario A

chromium repo
22,55s user 164,89s system 228% cpu 1:22,20 total

Scenario B

100k untracked files
14,37s user 7,52s system 106% cpu 20,501 total

Scenario D

vimfiles repo
6,93s user 8,00s system 79% cpu 18,668 total

For Scenario C, I'll have to recreate the repo first.

So comparing with #1046 (comment) the speed is a bit slower, but that is within testing invariance.

@dritter dritter added this to In progress in v0.7.0 via automation Nov 16, 2018

@diraol

This comment has been minimized.

Contributor

diraol commented Nov 16, 2018

Yup, I can confirm that this PR fixes #1067 ! =)
No more errors showing up in my terminal.

Thanks @macserv !

else
VCS_WORKDIR_HALF_DIRTY=false
# get the root for the current repo or submodule
local repoTopLevel="$(git rev-parse --show-toplevel 2> /dev/null)"

This comment has been minimized.

@dritter

dritter Nov 16, 2018

Collaborator
Suggested change Beta
local repoTopLevel="$(git rev-parse --show-toplevel 2> /dev/null)"
local repoTopLevel="$(command git rev-parse --show-toplevel 2> /dev/null)"

This comment has been minimized.

@macserv

macserv Nov 16, 2018

Nice, thanks for that.

p9k::set_default P9K_VCS_GIT_ALWAYS_SHOW_REMOTE_BRANCH false
function +vi-git-untracked() {
[[ -z "${vcs_comm[gitdir]}" || "${vcs_comm[gitdir]}" == "." ]] && return

This comment has been minimized.

@dritter

dritter Nov 16, 2018

Collaborator

Not sure if we want to keep this line. vcs_info runs anyway, thus we can bail out if vsc_info does not detect a git repo.

local untrackedFiles=$(command git ls-files --others --exclude-standard "${repoTopLevel}")
if [[ -z $untrackedFiles && "$P9K_VCS_SHOW_SUBMODULE_DIRTY" != "false" ]]; then

This comment has been minimized.

@dritter

dritter Nov 16, 2018

Collaborator
Suggested change Beta
if [[ -z $untrackedFiles && "$P9K_VCS_SHOW_SUBMODULE_DIRTY" != "false" ]]; then
if [[ -z $untrackedFiles && "$P9K_VCS_SHOW_SUBMODULE_DIRTY" == "true" ]]; then

Flipping the test is increases readability, IMHO.

This comment has been minimized.

@macserv

macserv Nov 16, 2018

I agree... it definitely improves readability and I’ll go ahead and commit this change. Let me walk you through my thinking on this, though, because I’m still a little bothered by a bigger question here. Is there a convention we want to follow for settings that use a string for a boolean value?

Requiring an exact match certainly works, as you suggest, and I did have it that way initially. I changed it because I followed the following thinking:

  • If the user sets a value themselves, they probably don’t want the default behavior
  • Since this is a boolean setting, there’s only one other behavior
  • If they’ve set any value, assume that's the one they want.
  • The default value is "false", so if the value is any different (e.g., "1", "on", "yes", "enable"), assume they want "true". (Note: this was erroneous, I know... originally, both settings were used in the file, and I assumed that we’d opt for faster processing by default.)

We might also consider having a function to handle this, which could return truth by actually evaluating the string against a set of true-ish and false-ish values (0/1, true/false, yes/no, on/off, enable(d)/disable(d)).

This comment has been minimized.

@dritter

dritter Nov 16, 2018

Collaborator

I already did my suggested changes directly in your branch. Sorry for fiddling in your branch, but I want to get this change out asap.

About boolean values and comparing them:
Hmm. Indeed I wrap boolean values in quotes by a force of habit. And it is a good idea to have a dedicated function that checks for multipe true-ish values. But this is out of scope for this PR.

P9K_VCS_SHOW_SUBMODULE_DIRTY defaulted to true not long ago. I flipped it because of performance reasons. But I am on the verge to let it default to true again, because this matches the default value of the past releases (only false in current master and next).

This comment has been minimized.

@macserv

macserv Nov 16, 2018

I already did my suggested changes directly in your branch. Sorry for fiddling in your branch, but I want to get this change out asap.

No worries! Appreciate it.

About boolean values and comparing them:
Hmm. Indeed I wrap boolean values in quotes by a force of habit. And it is a good idea to have a dedicated function that checks for multipe true-ish values. But this is out of scope for this PR.

Agreed... just food for thought.

P9K_VCS_SHOW_SUBMODULE_DIRTY defaulted to true not long ago. I flipped it because of performance reasons. But I am on the verge to let it default to true again, because this matches the default value of the past releases (only false in current master and next).

Sure. I’m personally fine with it either way, but it does slow things down noticeably if you’re working in a project like powerlevel9k or prezto, and you’ve got a lot of sub-sub-submodules, especially on the 2014 MBP that I use at work.

This comment has been minimized.

@dritter

dritter Nov 16, 2018

Collaborator

Sure. I’m personally fine with it either way, but it does slow things down noticeably if you’re working in a project like powerlevel9k or prezto, and you’ve got a lot of sub-sub-submodules, especially on the 2014 MBP that I use at work.

How bad is it? I am interested to gather some statistics about that. Could you redo the tests from #1046 (comment) on your machine?

@dritter dritter merged commit b430a21 into bhilburn:next Nov 16, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

v0.7.0 automation moved this from In progress to Done Nov 16, 2018

@dritter

This comment has been minimized.

Collaborator

dritter commented Nov 16, 2018

Thx @macserv . Merged into next. Would you mind porting your fix over to master?

@macserv

This comment has been minimized.

macserv commented Nov 16, 2018

I’d be happy to, but is it an issue on master? I only started having trouble when I switched to next.

@dritter

This comment has been minimized.

Collaborator

dritter commented Nov 16, 2018

Yes it is, because we backported the speed improvements to the vcs segment to master, without releasing a new version for now. With this backport we got the issue on there as well.

macserv pushed a commit to macserv/powerlevel9k that referenced this pull request Nov 17, 2018

dritter added a commit that referenced this pull request Nov 18, 2018

Merge pull request #1080 from macserv/port/vcs-segment-untracked-file…
…-check

[Bugfix] Port #1071 to `master` (Fix fatal errors emitted by untracked file check in vcs.zsh)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment