-
-
Notifications
You must be signed in to change notification settings - Fork 809
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
Make Get-GitBranch invoke git only once #472
Conversation
} | ||
|
||
dbg 'Inside git directory?' $sw |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you get rid of this dbg call and the next one (line 147)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't find it (or the other) useful anymore since it was no longer marking a part of the code that takes any time, but I'm happy to put it back if you prefer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to put it back; this dbg
is essentially replaced by the one on L68.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll let the other Keith comment on the correctness of the proposed change. He's the Git guru.
The one other edge case I can think of isn't correctly handled by posh-git today, so it's not really a regression, but this seems a reasonable time to address it. Branches can be symbolic references to other symbolic references, and
Git Bash correctly shows |
if (Test-Path $gitDir\rebase-merge\interactive) { | ||
dbg 'Found rebase-merge\interactive' $sw | ||
$r = '|REBASE-i' | ||
$b = "$(Get-Content $gitDir\rebase-merge\head-name)" | ||
$b = "$(Get-Content $gitDir\rebase-merge\head-name)" -replace 'refs/heads/', '' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't notice this before, but it's more correct to only replace that as a prefix: ^refs/heads/
.
Oh, I just looked at the Git Bash source for the first time and well, now I know why this code looked just precisely the way it did, huh. It looks like the Git Bash strategy is to not use rev-parse to emit the symbolic ref name in the common case, but instead just always read .git/HEAD (in addition to using rev-parse to find out the other things we need from it.) Then, instead of using the output of rev-parse to determine whether it was a detached HEAD and we need to describe (as this commit does) Git Bash looks at the contents of .git/HEAD to determine that, and either outputs the symbolic ref that it found in HEAD or tries describe. |
Yep, almost all of that code started as a direct port from the Bash prompt seven (!) years ago. Both Git and the Bash prompt have evolved a fair bit since then, and posh-git hasn't entirely kept up (e.g. #102). |
0cddf55
to
53c9145
Compare
I do appreciate your efforts at the time, though. Thanks! |
Previously, in the common case (in a git directory not in the middle of a rebase) it would invoke git separately for parsing the current ref (via
git symbolic-ref
) and determining whether we were in the git directory (viagit rev-parse
). Now it does those both at once viagit rev-parse
.Launching processes in Windows is kind of slow so this moved the needle a lot for me -- according to the debug timer, my prompt rendering time improved from ~50ms to ~30ms.
It's possible that there is a subtle difference between the output of
git symbolic-ref
andgit rev-parse
that someone might care about. (The obvious difference is that they have different behavior in the case that you're not on a symbolic ref; I addressed this.) I did not identify any and looking at the git source makes them look similar, albeit not identical.Tested the following cases:
I'm not sure I guessed 100% of the situations that the "parsing HEAD" fallback case was trying to cover, so someone who understands that might want to take a look and make sure I didn't break some functionality that someone was aiming for.