Fix Get-GitDirectory to work in bare repo. #370

rkeithhill · 2017-01-16T18:49:15Z

Fix #291. Also added Pester tests for this command.

dahlbyk · 2017-01-17T03:33:17Z

GitUtils.ps1

+            [System.IO.Path]::Combine($pathInfo.Path, '.git')
+        }
+        else {
+            $gitDir = git rev-parse --git-dir 2>$null


Is there any way to prevent this from lighting up $Error with the fatal messages? I can't find a rev-parse option to suppress the non-zero return code.

Interesting I don't see errors with this:

~\GitHub\temp\test.git [BARE:master]>

But in the past day or two I've seen non-terminating, error records appearing after a successful git command. I wonder if this is why git rev-parse was abandoned in the first place?

OTOH a non-zero exitcode should not cause PowerShell to display errors. Normally that happens with stderr output but we are redirecting that to $null.

Aha, you will get a NativeCommandError output above is someone has set $ErrorActionPreferencd to 'Stop'. Can you stick in a Write-Warning $ErrorActionPreference before the git rev-parse line and see what the value is?

$ErrorActionPreference is Continue

OK. If you open a new PowerShell instance do you still see the error?

FWIW, $ErrorActionPreference = 'Stop' will cause a problem if you call Get-GitDirectory directly outside of a git repo:

PS>Get-GitDirectory WARNING: Stop git : fatal: Not a git repository (or any of the parent directories): .git At C:\Users\Keith\GitHub\rkeithhill\posh-git\GitUtils.ps1:32 char:23 + $gitDir = git rev-parse --git-dir 2>$null + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + CategoryInfo : NotSpecified: (fatal: Not a gi...ectories): .git:String) [], RemoteException + FullyQualifiedErrorId : NativeCommandError

I can fix that by setting $ErrorActionPreference to Continue before calling git rev-parse.

This seems to adequately suppress the NativeCommandError:

$gitDir = cmd /c "git rev-parse --git-dir 2>nul"

We shouldn't have to resort to that. Let me keep looking.

If we do have to fiddle with $ErrorActionPreference, we could capture/reset that value in Invoke-Utf8ConsoleCommand. Should probably use that here anyway to correctly handle complex paths.

dahlbyk · 2017-01-17T03:50:04Z

test/Get-GitDirectory.Tests.ps1

+        It 'Returns correct path when under a child folder of the root of repo' {
+            $repoRoot = (Resolve-Path $PSScriptRoot\..).Path
+            Set-Location $PSScriptRoot
+            MakeNativePath (Get-GitDirectory) | Should BeExactly (Join-Path $repoRoot .git)


Any particular reason you changed up the MakeNativePath pattern here vs above?

Also, two more test ideas:

Set-Location $PSScriptRoot\.git

Set-Location $PSScriptRoot\.git\hooks

Because git rev-parse returns paths like C:/users/keith/... even on Windows. Hmm, maybe we should make Get-GitDirectory return a platform appropriate path. RE 1 & 2, I do test when path is root or posh-git repo and again when path is root\test. Remember in the test, $PSScriptRoot is the test dir. And I'm not too concerned with the Pester tests not being able to run on PS v2. Sound reasonable?

Hmm, maybe we should make Get-GitDirectory return a platform appropriate path.

This seems reasonable.

RE 1 & 2, I do test when path is root or posh-git repo and again when path is root\test.

Right, I'm mostly interested in making sure Get-GitDirectory correctly identifies the .git directory when we are inside that .git directory (at which point the prompt shows [GIT_DIR!]). Maybe I'm just paranoid.

Gotcha! I was testing under .git on the bare repo but not the normal repo. I"ll add some tests for that.

OK, added Get-GitBranch.Tests.ps1 to test those branch names.

It is easy to do a temporary/scoped override of $ErrorActionPreference.

Update Get-GitDirectory to use Invoke-Utf8ConsoleCommand to execute git rev-parse.

dahlbyk

For reference, #307 is another change motivated by reducing additions to $Error.

dahlbyk · 2017-01-17T15:04:51Z

Utils.ps1

@@ -22,6 +22,9 @@ Set-Alias ?? Invoke-NullCoalescing -Force
 function Invoke-Utf8ConsoleCommand([ScriptBlock]$cmd) {
    $currentEncoding = [Console]::OutputEncoding
    try {
+        # A native executable that writes to stderr AND has its stderr redirected will generate non-terminating
+        # error records if the user has set $ErrorActionPreference to Stop. Override that value in this scope.
+        $ErrorActionPreference = 'Continue'


This will only be scoped to this function?

I might not have been clear before... on PS 5.1.14393.693 I'm seeing an error record even with Continue:

That is correct PowerShell behavior. Even if we redirect the error stream to $null, it still gets captures in $Error e.g.:

10:47> $error.Clear() 11:54> Write-Error "kaboom" 2>$null 12:48> $error Write-Error "kaboom" 2>$null : kaboom + CategoryInfo : NotSpecified: (:) [Write-Error], WriteErrorException + FullyQualifiedErrorId : Microsoft.PowerShell.Commands.WriteErrorException

What we want to avoid is the error record being displayed without evaluating $Error:

24:38> $error.Clear() 25:28> $ErrorActionPreference = 'Stop' PS>Write-Error "kaboom" 2>$null Write-Error "kaboom" 2>$null : kaboom At line:1 char:1 + Write-Error "kaboom" 2>$null + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + CategoryInfo : NotSpecified: (:) [Write-Error], WriteErrorException + FullyQualifiedErrorId : Microsoft.PowerShell.Commands.WriteErrorException

Also, notice how my prompt function starts to fail, rendering PS> when I set $ErrorActionPreference to 'Stop'. This was done on master. When I switch to this branch, the prompt function does not fail for me, so that is good!

RE $ErrorActionPreference scoping, yes PowerShell does a "copy-on-write" of variables assigned to in a lower scopes that have the same name as a variable in a higher dynamic scope. This is why if you want to actually change the global value you have to use $global:ErrorActionPreference.

Even if we redirect the error stream to $null, it still gets captures in $Error e.g.:

And there's no way to suppress that? I'm not thrilled about polluting $Error with every prompt execution when not in a Git directory. Almost tempted to add code to Invoke-Utf8ConsoleCommand to clear out any new NativeCommandError entries in $Error.

Ironically it is the act of redirecting stderr to $null that causes this. Try it without the redirect and you'll see no updates to $Error. I'll look into this some more though.

The behavior of stderr also differs between hosts like PowerShell.exe and PowerShell_ISE.exe. In the former, the native app is given the console handles for stdout/err/in.

It feels like PowerShell tries to be a bit too clever about this, making it tough to use when we know exactly what we're trying to do. See also: #109.

Indeed! And #109 is what I was talking about - diff between console and ISE. I could go back to searching for .git the old way without calling git rev-parse. I just don't know how to identify a bare repo. Any tips?

I could go back to searching for .git the old way without calling git rev-parse. I just don't know how to identify a bare repo. Any tips?

Check for HEAD, refs or objects. Everything else is optional.

Maybe combine the approaches?

Check filesystem for .git or HEAD

If found, git rev-parse --git-dir to confirm

Else cd .. and goto 1

Asking my fellow PowerShell MVPs there is no good way around this. We can clean our errors out of $Error and we should probably stash them in a private module variable ($poshErrors?). There are ways to extract the value of private module variables IIRC. This might help in certain debugging scenarios.

Regarding the rewrite above A) I'm heading out to Disneyland tomorrow with the kids so I won't have a chance to look at this until Sunday night and B) I think we can avoid calling git rev-parse if we find .git right? That is essentially what the current approach does. OTOH if we find HEAD and refs dir, then we call git rev-parse. For the handling of $Error, we could put that in either the Invoke-Utf8ConsoleCommand or the prompt function? Thoughts?

Honestly, I think the best option is to "cheat" and use $gitDir = cmd /c "git rev-parse --git-dir 2>nul", unless there are performance concerns.

If we do go the $Error hack route, putting that in Invoke-Utf8ConsoleCommand seems best.

Using cmd increases the my prompt latency by about an order of magnitude (1-2 ms to 30ms) when outside a Git repo. Still that's not too bad. That said, we have other git.exe invocations that could cause the same issue seems like we should think about clearing those errors. Anyway I've updated this PR. Play with it and see how it does. Heading to airport now. :-)

dahlbyk · 2017-01-17T15:06:09Z

GitUtils.ps1

            if ($gitDir -eq '.') {
                # If in root or bare repo, we get '.' back
                $pathInfo.Path
            }
            else {
-                $gitDir
+                $gitDir -replace '\\|/', [System.IO.Path]::DirectorySeparatorChar


Would it make sense to use Resolve-Path here instead? That would also eliminate the need to handle . separately.

Sure we can simplify that. I guess I was being a bit perf paranoid. :-) But Resolve-Path . measures out to from 1 to 7 ms on my machine. Probably not enough to worry about.

That was causing the test failures.

If we find a file named HEAD and a dir named refs then we use rev-parse. Updated Invoke-Utf8ConsoleCommand to clear out any errors that it caused. Not done. Still need to take those errors and put them in a module variable but this will be useful for testing if we want to go this route.

rkeithhill · 2017-01-23T22:33:39Z

Hmm, those Pester test failures look like they shouldn't have failed. I'll look at that tonight.

Also make bare repo dir test a bit more strict.

rkeithhill · 2017-01-24T16:07:13Z

Looks like we may be hitting a bug in Pester 4.0 - pester/Pester#702

rkeithhill · 2017-01-24T16:33:47Z

@dahlbyk OK take a look at this and see what you think. BTW I loaded up the shipping posh-git and it spews quite a bit of stuff into $Error if you are in or under the .git dir.

dahlbyk

Just one question on $invokeErrors behavior, otherwise I think we're good here.

BTW I loaded up the shipping posh-git and it spews quite a bit of stuff into $Error if you are in or under the .git dir.

Definitely an edge case, but a nice bonus that we'll now be moving those errors elsewhere. Wouldn't be terrible to clean up those errors, as they seem to slow down the prompt (which should be faster, with less to do).

dahlbyk · 2017-01-25T16:04:36Z

Utils.ps1

+            $numNewErrors = $global:Error.Count - $errorCount
+            $invokeErrors.InsertRange(0, $global:Error.GetRange(0, $numNewErrors))
+            if ($invokeErrors.Count -gt 256) {
+                $invokeErrors.RemoveRange(0, ($invokeErrors.Count - 256))


A bit confused by this... wouldn't it make more sense to remove old errors from the bottom of the stack?

$invokeErrors.RemoveRange(256, ($invokeErrors.Count - 256))

No, you're not confused. I was being dumb. Sigh. Thanks for catching this.

rkeithhill · 2017-01-25T16:52:35Z

Wouldn't be terrible to clean up those errors

Yes. Perhap Get-GitStatus could check the branch name and if it is either \[BARE:[^\]]+\] or \[GIT_DIR!\] avoid calling git status?

dahlbyk · 2017-01-25T16:58:39Z

Perhap Get-GitStatus could check the branch name and if it is either \[BARE:[^\]]+\] or \[GIT_DIR!\] avoid calling git status?

For starters, we could check here if the current location starts with $gitDir.

dahlbyk

$invokeErrors adjustment works as expected. Merge as-is, or should we mitigate the obvious errors while we're thinking about it?

rkeithhill · 2017-01-25T17:07:14Z

Merge as-is, or should we mitigate the obvious errors while we're thinking about it?

This PR has gone on long enough I think. :-) Let's fix that issue in a new PR.

rkeithhill · 2017-01-25T17:15:27Z

BTW do you prefer squash merge, rebase merge or a simple merge on this one?

dahlbyk · 2017-01-25T17:36:36Z

BTW do you prefer squash merge, rebase merge or a simple merge on this one?

Either simple merge or (local rebase + force push + simple merge). Wish they had "rebase then merge".

rkeithhill · 2017-01-25T17:37:29Z

They do have "Rebase and merge" (in the drop down). BTW I went ahead and added that check for in .git or bare repo. Could you look at that last commit?

rkeithhill · 2017-01-25T17:41:32Z

Oops, with that change I need to update some of the tests.

dahlbyk

They do have "Rebase and merge" (in the drop down).

They do, but it doesn't create a merge commit. If I'm going to rebase, I want history to look like this:


-*---------*---*------*-
  \       / \ / \    /
   *--*--*   *   *--*

BTW I went ahead and added that check for in .git or bare repo. Could you look at that last commit?

Change looks good, but build failed.

rkeithhill · 2017-01-25T17:50:28Z

I'll look at those test failures tonight. BTW I thought with Git you weren't supposed to rebase anything that is public i.e. pushed to a public Git repo? Also, you are not talking about an interactive rebase (with squashing), right? You are talking about rebasing the branch onto master.

rkeithhill · 2017-01-25T18:04:15Z

@dahlbyk Tests pass now. I've got to head out. If you want to merge this, go for it. Otherwise, I can do it later tonight.

dahlbyk · 2017-01-25T18:34:38Z

BTW I thought with Git you weren't supposed to rebase anything that is public i.e. pushed to a public Git repo?

This is where things get philosophical. Some people have a firm "never push --force" rule. I'm not one of those people. (Actually, I am, but only because Git added push --force-with-lease). When people say "don't rebase anything that's public" what they really mean is "don't rebase anything people are depending on because things can get scary." Note "scary" and not "broken" - I have never encountered an upstream rebase that I couldn't handle locally.

If this branch had been around for months and we had referred people here to test something out (e.g. #208), it might be reasonable to treat as history that shouldn't be rewritten. And if you and I were working together on the same branch, we would want to communicate carefully if a rebase was necessary. But once a feature is ready to ship, I'm firmly in the "clean history is best history" camp. I used to consider a linear history cleanest (git log --graph --oneline c481e5b) but have since come to prefer rebased PRs with a merge commit.

Also, you are not talking about an interactive rebase (with squashing), right?

Correct, I'm talking about the GitHub merge-via-rebase button. Since "clean history is best history", a rebase --interactive with selective squashing (e.g. combining a1bb28f into its parent) would be acceptable (preferred, actually, for my private work). A year from now there's zero value in the implementation detail of knowing that CI failed because tests needed to be adjusted - the test changes logically belong with the change in functionality.

That being said, I have come to terms with posh-git history being a mess, so this type of clean-up is strictly optional.

You are talking about rebasing the branch onto master.

Yeah, this is what the GitHub merge-via-rebase button does. Where master is whatever branch the PR targets, of course.

dahlbyk · 2017-01-25T18:35:44Z

rebase wall of text aside, you are welcome to clean up history here if you wish, or merge straight-away. Choose wisely. 😜

rkeithhill · 2017-01-25T18:41:34Z

Thanks for your insights on rebase, much appreciated. I'll do the rebasing tonight. BTW is there a preferred ordered between interactive rebase and rebase? I would probably do the interactive first and then rebase onto master.

dahlbyk · 2017-01-25T18:54:54Z

BTW is there a preferred ordered between interactive rebase and rebase? I would probably do the interactive first and then rebase onto master.

I most often just git rebase -i master (after making sure master is up-to-date!), which does both at once. If I encounter unexpected conflicts, I'll do an in-place rewrite first (git rebase --interactive $(git merge-base master HEAD)) then rebase the cleaned-up commits onto master.

Relevant thread that should really be a blog post/series: https://twitter.com/dahlbyk/status/745296644590043136

rkeithhill added 2 commits January 16, 2017 11:47

Fix Get-GitDirectory to work in bare repo.

e84ce2f

Fix #291. Also added Pester tests for this command.

Remove duplicate test.

b923fa1

dahlbyk reviewed Jan 17, 2017

View reviewed changes

rkeithhill added 4 commits January 16, 2017 21:16

Handle known issue w/EAPreference set to Stop.

bb8058d

It is easy to do a temporary/scoped override of $ErrorActionPreference.

Add Get-GitBranch Pester tests.

e97f38a

Get-GitDirectory returns platform approp paths

e857c2a

Update Utf8ConsoleCommand to set EAPreference.

99829c5

Update Get-GitDirectory to use Invoke-Utf8ConsoleCommand to execute git rev-parse.

dahlbyk reviewed Jan 17, 2017

View reviewed changes

rkeithhill added 5 commits January 17, 2017 10:27

Use Resolve-Path on $gitDir to simplify script.

4d95039

Merging this file on GitHub too painful.

7b56374

Merge branch 'master' into rkeithhill/is291-fix-bare-repo

38bc22a

Change EOL back to CRLF.

67ea721

That was causing the test failures.

Store error records in module variable.

3a986ef

Also make bare repo dir test a bit more strict.

Work around Pester bug.

db10612

dahlbyk reviewed Jan 25, 2017

View reviewed changes

Remove old errors from end of invokeErrors list.

8887fa4

dahlbyk approved these changes Jan 25, 2017

View reviewed changes

Don't call git status when in .git or bare repo.

a27044e

dahlbyk requested changes Jan 25, 2017

View reviewed changes

Fix Pester test failures.

a1bb28f

dahlbyk approved these changes Jan 25, 2017

View reviewed changes

rkeithhill merged commit 87e6832 into dahlbyk:master Jan 26, 2017

rkeithhill deleted the rkeithhill/is291-fix-bare-repo branch January 26, 2017 02:53

Fix Get-GitDirectory to work in bare repo. #370

Fix Get-GitDirectory to work in bare repo. #370

Conversation

rkeithhill commented Jan 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkeithhill Jan 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dahlbyk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkeithhill commented Jan 23, 2017

rkeithhill commented Jan 24, 2017

rkeithhill commented Jan 24, 2017

dahlbyk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkeithhill commented Jan 25, 2017

dahlbyk commented Jan 25, 2017

dahlbyk left a comment

Choose a reason for hiding this comment

rkeithhill commented Jan 25, 2017

rkeithhill commented Jan 25, 2017

dahlbyk commented Jan 25, 2017

rkeithhill commented Jan 25, 2017 • edited Loading

rkeithhill commented Jan 25, 2017

dahlbyk left a comment

Choose a reason for hiding this comment

rkeithhill commented Jan 25, 2017

rkeithhill commented Jan 25, 2017

dahlbyk commented Jan 25, 2017

dahlbyk commented Jan 25, 2017

rkeithhill commented Jan 25, 2017

dahlbyk commented Jan 25, 2017

rkeithhill Jan 17, 2017 •

edited

Loading

rkeithhill commented Jan 25, 2017 •

edited

Loading