[Bugfix] Fix errors emitted by VCS segment's untracked file check when in a submodule #1071
Conversation
…nts, and resolve double-set `P9K_VCS_SHOW_SUBMODULE_DIRTY` default in favor of the current documentation.
…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.
|
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 Achromium repo Scenario B100k untracked files Scenario Dvimfiles repo 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. |
segments/vcs.p9k
Outdated
| 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)" |
There was a problem hiding this comment.
| local repoTopLevel="$(git rev-parse --show-toplevel 2> /dev/null)" | |
| local repoTopLevel="$(command git rev-parse --show-toplevel 2> /dev/null)" |
segments/vcs.p9k
Outdated
| p9k::set_default P9K_VCS_GIT_ALWAYS_SHOW_REMOTE_BRANCH false | ||
|
|
||
| function +vi-git-untracked() { | ||
| [[ -z "${vcs_comm[gitdir]}" || "${vcs_comm[gitdir]}" == "." ]] && return |
There was a problem hiding this comment.
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.
segments/vcs.p9k
Outdated
|
|
||
| local untrackedFiles=$(command git ls-files --others --exclude-standard "${repoTopLevel}") | ||
|
|
||
| if [[ -z $untrackedFiles && "$P9K_VCS_SHOW_SUBMODULE_DIRTY" != "false" ]]; then |
There was a problem hiding this comment.
| 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.
There was a problem hiding this comment.
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)).
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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_DIRTYdefaulted totruenot long ago. I flipped it because of performance reasons. But I am on the verge to let it default totrueagain, because this matches the default value of the past releases (onlyfalsein currentmasterandnext).
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.
There was a problem hiding this comment.
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?
|
Thx @macserv . Merged into |
|
I’d be happy to, but is it an issue on |
|
Yes it is, because we backported the speed improvements to the vcs segment to |
… file check in vcs.zsh)
…-check [Bugfix] Port #1071 to `master` (Fix fatal errors emitted by untracked file check in vcs.zsh)
P9K_VCS_SHOW_SUBMODULE_DIRTYdefault in favor of the current documentation.VCS_WORKDIR_HALF_DIRTYstatus reflects the untracked file state from the current repo down instead of always going back to the highest-level repo..gitfolder (it's not considered to be a part of the working tree).VCS_WORKDIR_HALF_DIRTYstatus can be established by the presence of untracked files in the current repo's tree.